Repository: scrapy/scrapy
Pull request #353 (2013-07-19 17:10:26)

[MRG] Item multi inheritance fix

chekunkov: What is missing in this PR? Can it be merged?
dangra: @chekunkov anything to comment on its implementation? 

From my past comments I still don't get a test case for defining and/or overriding fields of its parents.
nramirezuy: @dangra https://github.com/scrapy/scrapy/pull/353/files#diff-b610b6c1bd7114fdb73120bf028608caR169 ?

Patch:
diff --git a/scrapy/crawler.py b/scrapy/crawler.py
index b4706919a..dab1043ec 100644
--- a/scrapy/crawler.py
+++ b/scrapy/crawler.py
@@ -192,8 +192,8 @@ def _get_spider_loader(settings):
             'Please use SPIDER_LOADER_CLASS.',
             category=ScrapyDeprecationWarning, stacklevel=2
         )
-    cls_path = settings.get('SPIDER_MANAGER_CLASS',
-                            settings.get('SPIDER_LOADER_CLASS'))
+    cls_path = settings.get('SPIDER_LOADER_CLASS',
+                            settings.get('SPIDER_MANAGER_CLASS'))
     loader_cls = load_object(cls_path)
     verifyClass(ISpiderLoader, loader_cls)
     return loader_cls.from_settings(settings.frozencopy())
diff --git a/scrapy/item.py b/scrapy/item.py
index 9998010b2..8ac279642 100644
--- a/scrapy/item.py
+++ b/scrapy/item.py
@@ -24,18 +24,21 @@ class Field(dict):
 class ItemMeta(ABCMeta):
 
     def __new__(mcs, class_name, bases, attrs):
+        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)
+
         fields = {}
         new_attrs = {}
-        for n, v in six.iteritems(attrs):
+        for n in dir(_class):
+            v = getattr(_class, n)
             if isinstance(v, Field):
                 fields[n] = v
-            else:
-                new_attrs[n] = v
+            elif n in attrs:
+                new_attrs[n] = attrs[n]
 
-        cls = super(ItemMeta, mcs).__new__(mcs, class_name, bases, new_attrs)
-        cls.fields = cls.fields.copy()
-        cls.fields.update(fields)
-        return cls
+        new_attrs['fields'] = fields
+        new_attrs['_class'] = _class
+        return super(ItemMeta, mcs).__new__(mcs, class_name, bases, new_attrs)
 
 
 class DictItem(MutableMapping, BaseItem):


Test patch:
diff --git a/tests/test_crawler.py b/tests/test_crawler.py
index 6a282ed84..50da01e73 100644
--- a/tests/test_crawler.py
+++ b/tests/test_crawler.py
@@ -5,7 +5,6 @@ from zope.interface.verify import DoesNotImplement
 
 from scrapy.crawler import Crawler, CrawlerRunner
 from scrapy.settings import Settings, default_settings
-from scrapy.spiderloader import SpiderLoader
 from scrapy.utils.spider import DefaultSpider
 from scrapy.utils.misc import load_object
 
@@ -61,10 +60,6 @@ class SpiderLoaderWithWrongInterface(object):
         pass
 
 
-class CustomSpiderLoader(SpiderLoader):
-    pass
-
-
 class CrawlerRunnerTestCase(unittest.TestCase):
 
     def test_spider_manager_verify_interface(self):
@@ -92,13 +87,3 @@ class CrawlerRunnerTestCase(unittest.TestCase):
             self.assertIn("CrawlerRunner.spider_loader", str(w[0].message))
             sl_cls = load_object(runner.settings['SPIDER_LOADER_CLASS'])
             self.assertIsInstance(spiders, sl_cls)
-
-    def test_spidermanager_deprecation(self):
-        with warnings.catch_warnings(record=True) as w:
-            runner = CrawlerRunner({
-                'SPIDER_MANAGER_CLASS': 'tests.test_crawler.CustomSpiderLoader'
-            })
-            self.assertIsInstance(runner.spider_loader, CustomSpiderLoader)
-            self.assertEqual(len(w), 1)
-            self.assertIn('Please use SPIDER_LOADER_CLASS', str(w[0].message))
-
diff --git a/tests/test_item.py b/tests/test_item.py
index 5de77d22e..76061bbd9 100644
--- a/tests/test_item.py
+++ b/tests/test_item.py
@@ -134,6 +134,76 @@ class ItemTest(unittest.TestCase):
         self.assertEqual(list(i.keys()), ['keys'])
         self.assertEqual(list(i.values()), [3])
 
+    def test_metaclass_multiple_inheritance_simple(self):
+        class A(Item):
+            save = Field(default='A')
+
+        class B(A): pass
+
+        class C(Item):
+            save = Field(default='C')
+
+        class D(B, C): pass
+
+        self.assertEqual(D(save='X')['save'], 'X')
+        self.assertEqual(D.fields, {'save': {'default': 'A'}})
+
+        # D class inverted
+        class E(C, B): pass
+
+        self.assertEqual(E(save='X')['save'], 'X')
+        self.assertEqual(E.fields, {'save': {'default': 'C'}})
+
+    def test_metaclass_multiple_inheritance_diamond(self):
+        class A(Item):
+            save = Field(default='A')
+            load = Field(default='A')
+
+        class B(A): pass
+
+        class C(A):
+            save = Field(default='C')
+
+        class D(B, C):
+            load = Field(default='D')
+
+        self.assertEqual(D(save='X')['save'], 'X')
+        self.assertEqual(D(load='X')['load'], 'X')
+        self.assertEqual(D.fields, {'save': {'default': 'C'},
+            'load': {'default': 'D'}})
+
+        # D class inverted
+        class E(C, B):
+            load = Field(default='E')
+
+        self.assertEqual(E(save='X')['save'], 'X')
+        self.assertEqual(E(load='X')['load'], 'X')
+        self.assertEqual(E.fields, {'save': {'default': 'C'},
+            'load': {'default': 'E'}})
+
+    def test_metaclass_multiple_inheritance_without_metaclass(self):
+        class A(Item):
+            save = Field(default='A')
+
+        class B(A): pass
+
+        class C(object):
+            not_allowed = Field(default='not_allowed')
+            save = Field(default='C')
+
+        class D(B, C): pass
+
+        self.assertRaises(KeyError, D, not_allowed='value')
+        self.assertEqual(D(save='X')['save'], 'X')
+        self.assertEqual(D.fields, {'save': {'default': 'A'}})
+
+        # D class inverted
+        class E(C, B): pass
+
+        self.assertRaises(KeyError, E, not_allowed='value')
+        self.assertEqual(E(save='X')['save'], 'X')
+        self.assertEqual(E.fields, {'save': {'default': 'A'}})
+
     def test_to_dict(self):
         class TestItem(Item):
             name = Field()