Repository: scrapy/scrapy
Pull request #2509 (2017-01-24 14:31:39)

[MRG+1] BUG: Fix __classcell__ propagation required in Python 3.6.
Fixes https://github.com/scrapy/scrapy/issues/2511

Python 3.6 added simpler customization of class creation but this
requires to propagate correctly the __classcell__ attribute in custom
__new__ methods.

See https://docs.python.org/3.6/whatsnew/3.6.html#pep-487-simpler-customization-of-class-creation

Related:
https://github.com/django/django/pull/7653
https://bugs.python.org/issue23722

rolando: This aims to fix https://github.com/scrapy-plugins/scrapy-djangoitem/issues/18
codecov-io: ## [Current coverage](https://codecov.io/gh/scrapy/scrapy/pull/2509?src=pr) is 83.44% (diff: 33.33%)
> Merging [#2509](https://codecov.io/gh/scrapy/scrapy/pull/2509?src=pr) into [master](https://codecov.io/gh/scrapy/scrapy/branch/master?src=pr) will decrease coverage by **0.01%**

```diff
@@             master      #2509   diff @@
==========================================
  Files           161        161          
  Lines          8780       8783     +3   
  Methods           0          0          
  Messages          0          0          
  Branches       1288       1289     +1   
==========================================
+ Hits           7328       7329     +1   
- Misses         1204       1205     +1   
- Partials        248        249     +1   
```

> Powered by [Codecov](https://codecov.io?src=pr). Last update [4620d2f...a86281d](https://codecov.io/gh/scrapy/scrapy/compare/4620d2f4256093296c490b412a95e4eb9aa5bd41...a86281d4f0c7f1ff2fcad49a34c9cd72c1746abc?src=pr)
dangra: +1

was the regression catch by Scrapy test suite? If not, I guess we need to add a test case before merging.
redapple: No, it wasn't caught by Scrapy's test suite.
When Python 3.6 build was added to Travis, [everything was green](https://github.com/scrapy/scrapy/pull/2485).
So +1 to a test on scrapy.Item custimization
rolando: OK!

Patch:
diff --git a/scrapy/item.py b/scrapy/item.py
index 138728a9a..aa05e9c69 100644
--- a/scrapy/item.py
+++ b/scrapy/item.py
@@ -25,6 +25,7 @@ class Field(dict):
 class ItemMeta(ABCMeta):
 
     def __new__(mcs, class_name, bases, attrs):
+        classcell = attrs.pop('__classcell__', None)
         new_bases = tuple(base._class for base in bases if hasattr(base, '_class'))
         _class = super(ItemMeta, mcs).__new__(mcs, 'x_' + class_name, new_bases, attrs)
 
@@ -39,6 +40,8 @@ class ItemMeta(ABCMeta):
 
         new_attrs['fields'] = fields
         new_attrs['_class'] = _class
+        if classcell is not None:
+            new_attrs['__classcell__'] = classcell
         return super(ItemMeta, mcs).__new__(mcs, class_name, bases, new_attrs)
 
 


Test patch:
diff --git a/tests/test_item.py b/tests/test_item.py
index dcb169c3a..85a554de0 100644
--- a/tests/test_item.py
+++ b/tests/test_item.py
@@ -1,8 +1,14 @@
+import sys
 import unittest
 
-from scrapy.item import Item, Field
 import six
 
+from scrapy.item import ABCMeta, Item, ItemMeta, Field
+from tests import mock
+
+
+PY36_PLUS = (sys.version_info.major >= 3) and (sys.version_info.minor >= 6)
+
 
 class ItemTest(unittest.TestCase):
 
@@ -244,5 +250,49 @@ class ItemTest(unittest.TestCase):
         self.assertNotEqual(item['name'], copied_item['name'])
 
 
+class ItemMetaTest(unittest.TestCase):
+
+    def test_new_method_propagates_classcell(self):
+        new_mock = mock.Mock(side_effect=ABCMeta.__new__)
+        base = ItemMeta.__bases__[0]
+
+        with mock.patch.object(base, '__new__', new_mock):
+
+            class MyItem(Item):
+                if not PY36_PLUS:
+                    # This attribute is an internal attribute in Python 3.6+
+                    # and must be propagated properly. See
+                    # https://docs.python.org/3.6/reference/datamodel.html#creating-the-class-object
+                    # In <3.6, we add a dummy attribute just to ensure the
+                    # __new__ method propagates it correctly.
+                    __classcell__ = object()
+
+                def f(self):
+                    # For rationale of this see:
+                    # https://github.com/python/cpython/blob/ee1a81b77444c6715cbe610e951c655b6adab88b/Lib/test/test_super.py#L222
+                    return __class__
+
+            MyItem()
+
+        (first_call, second_call) = new_mock.call_args_list[-2:]
+
+        mcs, class_name, bases, attrs = first_call[0]
+        assert '__classcell__' not in attrs
+        mcs, class_name, bases, attrs = second_call[0]
+        assert '__classcell__' in attrs
+
+
+class ItemMetaClassCellRegression(unittest.TestCase):
+
+    def test_item_meta_classcell_regression(self):
+        class MyItem(six.with_metaclass(ItemMeta, Item)):
+            def __init__(self, *args, **kwargs):
+                # This call to super() trigger the __classcell__ propagation
+                # requirement. When not done properly raises an error:
+                # TypeError: __class__ set to <class '__main__.MyItem'>
+                # defining 'MyItem' as <class '__main__.MyItem'>
+                super(MyItem, self).__init__(*args, **kwargs)
+
+
 if __name__ == "__main__":
     unittest.main()