Repository: scrapy/scrapy
Pull request #1228 (2015-05-13 16:28:34)

item fields defined on fields class attribute fix
reported on: #1217 

dangra: Do we really want to support defining fields like this in ItemMeta? 

The user says it works for DictItem which is fine because it is the case it must work, but for Item (and ItemMeta) it was working as a side effect, even untested.

In any case if we want to support defining fields mixed in `fields` attribute plus Field's attributes, let's add a simple test case without inheritance involved.
nramirezuy: It was working before and was fair easy to fix it. 
I defined an item that way, the only functionality I can think of is consistency with another side effect :tongue: 

```python
>>> class A(Item):
...     fields = {'save': Field(default='A')}
...     load = Field()
... 
>>> a = A()
>>> a.fields
{'load': {}, 'save': {'default': 'A'}}
>>> A.new = Field()
>>> a.fields
{'load': {}, 'save': {'default': 'A'}}
>>> A.fields['new_field'] = Field()
>>> a.fields
{'load': {}, 'new_field': {}, 'save': {'default': 'A'}}
>>> a['new_field'] = 1
>>> a
{'new_field': 1}
```
dangra: Ok, what do you think about adding a very simple test case without inheritance ?  
nramirezuy: @dangra That works?
dangra: IMO is much easier to debug a broken test when it test a single case, even better if new cases doesn't modify previous well defined tests.

I would add a `test_metaclass_with_fields_attribute()` instead of modified `test_metaclass()`.

Patch:
diff --git a/docs/_ext/scrapydocs.py b/docs/_ext/scrapydocs.py
index f0827f2b1..1fa1c93d6 100644
--- a/docs/_ext/scrapydocs.py
+++ b/docs/_ext/scrapydocs.py
@@ -1,82 +1,5 @@
 from docutils.parsers.rst.roles import set_classes
 from docutils import nodes
-from sphinx.util.compat import Directive
-from sphinx.util.nodes import make_refnode
-from operator import itemgetter
-
-
-class settingslist_node(nodes.General, nodes.Element):
-    pass
-
-
-class SettingsListDirective(Directive):
-    def run(self):
-        return [settingslist_node('')]
-
-
-def is_setting_index(node):
-    if node.tagname == 'index':
-        # index entries for setting directives look like:
-        # [(u'pair', u'SETTING_NAME; setting', u'std:setting-SETTING_NAME', '')]
-        entry_type, info, refid, _ = node['entries'][0]
-        return entry_type == 'pair' and info.endswith('; setting')
-    return False
-
-
-def get_setting_target(node):
-    # target nodes are placed next to the node in the doc tree
-    return node.parent[node.parent.index(node) + 1]
-
-
-def get_setting_name_and_refid(node):
-    """Extract setting name from directive index node"""
-    entry_type, info, refid, _ = node['entries'][0]
-    return info.replace('; setting', ''), refid
-
-
-def collect_scrapy_settings_refs(app, doctree):
-    env = app.builder.env
-
-    if not hasattr(env, 'scrapy_all_settings'):
-        env.scrapy_all_settings = []
-
-    for node in doctree.traverse(is_setting_index):
-        targetnode = get_setting_target(node)
-        assert isinstance(targetnode, nodes.target), "Next node is not a target"
-
-        setting_name, refid = get_setting_name_and_refid(node)
-
-        env.scrapy_all_settings.append({
-            'docname': env.docname,
-            'setting_name': setting_name,
-            'refid': refid,
-        })
-
-
-def make_setting_element(setting_data, app, fromdocname):
-    refnode = make_refnode(app.builder, fromdocname,
-                           todocname=setting_data['docname'],
-                           targetid=setting_data['refid'],
-                           child=nodes.Text(setting_data['setting_name']))
-    p = nodes.paragraph()
-    p += refnode
-
-    item = nodes.list_item()
-    item += p
-    return item
-
-
-def replace_settingslist_nodes(app, doctree, fromdocname):
-    env = app.builder.env
-
-    for node in doctree.traverse(settingslist_node):
-        settings_list = nodes.bullet_list()
-        settings_list.extend([make_setting_element(d, app, fromdocname)
-                              for d in sorted(env.scrapy_all_settings,
-                                              key=itemgetter('setting_name'))
-                              if fromdocname != d['docname']])
-        node.replace_self(settings_list)
-
 
 def setup(app):
     app.add_crossref_type(
@@ -104,12 +27,6 @@ def setup(app):
     app.add_role('issue', issue_role)
     app.add_role('rev', rev_role)
 
-    app.add_node(settingslist_node)
-    app.add_directive('settingslist', SettingsListDirective)
-
-    app.connect('doctree-read', collect_scrapy_settings_refs)
-    app.connect('doctree-resolved', replace_settingslist_nodes)
-
 def source_role(name, rawtext, text, lineno, inliner, options={}, content=[]):
     ref = 'https://github.com/scrapy/scrapy/blob/master/' + text
     set_classes(options)
diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst
index a72a991e5..26a6d762d 100644
--- a/docs/topics/settings.rst
+++ b/docs/topics/settings.rst
@@ -1015,16 +1015,6 @@ Default: ``"Scrapy/VERSION (+http://scrapy.org)"``
 
 The default User-Agent to use when crawling, unless overridden.
 
-
-Settings documented elsewhere:
-------------------------------
-
-The following settings are documented elsewhere, please check each specific
-case to see how to enable and use them.
-
-.. settingslist::
-
-
 .. _Amazon web services: http://aws.amazon.com/
 .. _breadth-first order: http://en.wikipedia.org/wiki/Breadth-first_search
 .. _depth-first order: http://en.wikipedia.org/wiki/Depth-first_search
diff --git a/scrapy/item.py b/scrapy/item.py
index 8ac279642..138728a9a 100644
--- a/scrapy/item.py
+++ b/scrapy/item.py
@@ -6,6 +6,7 @@ See documentation in docs/topics/item.rst
 
 from pprint import pformat
 from collections import MutableMapping
+
 from abc import ABCMeta
 import six
 
@@ -27,7 +28,7 @@ class ItemMeta(ABCMeta):
         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 = {}
+        fields = getattr(_class, 'fields', {})
         new_attrs = {}
         for n in dir(_class):
             v = getattr(_class, n)


Test patch:
diff --git a/tests/test_item.py b/tests/test_item.py
index 76061bbd9..dcb169c3a 100644
--- a/tests/test_item.py
+++ b/tests/test_item.py
@@ -120,6 +120,14 @@ class ItemTest(unittest.TestCase):
         self.assertSortedEqual(list(i.keys()), ['keys', 'values', 'name'])
         self.assertSortedEqual(list(i.values()), [u'Keys', u'Values', u'John'])
 
+    def test_metaclass_with_fields_attribute(self):
+        class TestItem(Item):
+            fields = {'new': Field(default='X')}
+
+        item = TestItem(new=u'New')
+        self.assertSortedEqual(list(item.keys()), ['new'])
+        self.assertSortedEqual(list(item.values()), [u'New'])
+
     def test_metaclass_inheritance(self):
         class BaseItem(Item):
             name = Field()
@@ -136,41 +144,51 @@ class ItemTest(unittest.TestCase):
 
     def test_metaclass_multiple_inheritance_simple(self):
         class A(Item):
+            fields = {'load': Field(default='A')}
             save = Field(default='A')
 
         class B(A): pass
 
         class C(Item):
+            fields = {'load': Field(default='C')}
             save = Field(default='C')
 
         class D(B, C): pass
 
-        self.assertEqual(D(save='X')['save'], 'X')
-        self.assertEqual(D.fields, {'save': {'default': 'A'}})
+        item = D(save='X', load='Y')
+        self.assertEqual(item['save'], 'X')
+        self.assertEqual(item['load'], 'Y')
+        self.assertEqual(D.fields, {'load': {'default': 'A'},
+            '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'}})
+        self.assertEqual(E(load='X')['load'], 'X')
+        self.assertEqual(E.fields, {'load': {'default': 'C'},
+            'save': {'default': 'C'}})
 
     def test_metaclass_multiple_inheritance_diamond(self):
         class A(Item):
+            fields = {'update': Field(default='A')}
             save = Field(default='A')
             load = Field(default='A')
 
         class B(A): pass
 
         class C(A):
+            fields = {'update': Field(default='C')}
             save = Field(default='C')
 
         class D(B, C):
+            fields = {'update': Field(default='D')}
             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'}})
+            'load': {'default': 'D'}, 'update': {'default': 'D'}})
 
         # D class inverted
         class E(C, B):
@@ -179,15 +197,17 @@ class ItemTest(unittest.TestCase):
         self.assertEqual(E(save='X')['save'], 'X')
         self.assertEqual(E(load='X')['load'], 'X')
         self.assertEqual(E.fields, {'save': {'default': 'C'},
-            'load': {'default': 'E'}})
+            'load': {'default': 'E'}, 'update': {'default': 'C'}})
 
     def test_metaclass_multiple_inheritance_without_metaclass(self):
         class A(Item):
+            fields = {'load': Field(default='A')}
             save = Field(default='A')
 
         class B(A): pass
 
         class C(object):
+            fields = {'load': Field(default='C')}
             not_allowed = Field(default='not_allowed')
             save = Field(default='C')
 
@@ -195,14 +215,16 @@ class ItemTest(unittest.TestCase):
 
         self.assertRaises(KeyError, D, not_allowed='value')
         self.assertEqual(D(save='X')['save'], 'X')
-        self.assertEqual(D.fields, {'save': {'default': 'A'}})
+        self.assertEqual(D.fields, {'save': {'default': 'A'},
+            'load': {'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'}})
+        self.assertEqual(E.fields, {'save': {'default': 'A'},
+            'load': {'default': 'A'}})
 
     def test_to_dict(self):
         class TestItem(Item):