Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Item multi inheritance fix #353

Merged
merged 1 commit into from Apr 22, 2015
Merged

Conversation

@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Jul 19, 2013

No description provided.

@dangra
dangra reviewed Jul 22, 2013
View changes
scrapy/item.py Outdated
@@ -33,6 +33,9 @@ def __new__(mcs, class_name, bases, attrs):
cls = type.__new__(mcs, class_name, bases, new_attrs)
cls.fields = cls.fields.copy()
cls.fields.update(fields)
for base in bases:
fields = base.fields.copy()
cls.fields.update(fields)

This comment has been minimized.

@dangra

dangra Jul 22, 2013
Member

dict.update() already does a shallow copy of base.fields, no need to copy() it.

and bases can be non-Item too, think on mixins not extending scrapy.item.Item class.

This comment has been minimized.

@nramirezuy

nramirezuy Jul 22, 2013
Author Contributor

If I remove the copies the tests fails, I'm not sure why.

keys = Field()
values = Field()

class FinalItem(BaseItem, NameItem):

This comment has been minimized.

@dangra

dangra Dec 16, 2013
Member

This lacks a test for a field that exists in base classes and it is redefined in FinalItem.


i = FinalItem(keys=3, name='Manolo')
self.assertEqual(i['keys'], 3)
self.assertEqual(i['name'], 'Manolo')

This comment has been minimized.

@dangra

dangra Dec 16, 2013
Member

it misses values field test

@@ -33,6 +33,10 @@ def __new__(mcs, class_name, bases, attrs):
cls = type.__new__(mcs, class_name, bases, new_attrs)
cls.fields = cls.fields.copy()
cls.fields.update(fields)
for base in bases:
if issubclass(base, DictItem):

This comment has been minimized.

@dangra

dangra Dec 16, 2013
Member

missing a unit test case for mixed base classes that are not DictItem subclasses.

@nramirezuy
Copy link
Contributor Author

@nramirezuy nramirezuy commented Jul 24, 2014

@dangra Tests added and a behavior fix. Do you want to add the @arijitchakraborty SEP from #487 to the sep folder?

for base in bases:
if issubclass(base, DictItem):
fields = base.fields.copy()
fields.update(fields)

This comment has been minimized.

@dangra

dangra Jul 30, 2014
Member

How is this supposed to work? it updates fields with fields and opaques the declaration of fields in line 26

@chekunkov
Copy link
Contributor

@chekunkov chekunkov commented Apr 20, 2015

What is missing in this PR? Can it be merged?

@dangra
Copy link
Member

@dangra dangra commented Apr 20, 2015

@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.

@@ -24,18 +24,20 @@ class Field(dict):
class ItemMeta(ABCMeta):

def __new__(mcs, class_name, bases, attrs):
_class = super(ItemMeta, mcs).__new__(mcs, 'x_' + class_name, tuple(base._class for base in bases if hasattr(base, '_class')), attrs)

This comment has been minimized.

@kmike

kmike Apr 21, 2015
Member

hey @nramirezuy - could you please split it into multiple lines?
e.g. tuple(base._class for base in bases if hasattr(base, '_class')) is a good candidate for a local variable.

This comment has been minimized.

@nramirezuy

nramirezuy Apr 21, 2015
Author Contributor

done 😄

@nramirezuy nramirezuy force-pushed the nramirezuy:item-multi_inherit branch from 0010e3a to 49a2cc9 Apr 21, 2015
class D(B, C): pass

self.assertEqual(D(save='X')['save'], 'X')
self.assertEqual(D.fields, {'save': {'default': 'C'}})

This comment has been minimized.

@dangra

dangra Apr 21, 2015
Member

I think it lacks another testcase where class D overrides a field defined in class A or B.

This comment has been minimized.

@nramirezuy

nramirezuy Apr 21, 2015
Author Contributor

Done for D and E

@nramirezuy nramirezuy force-pushed the nramirezuy:item-multi_inherit branch from 49a2cc9 to 7871acd Apr 21, 2015
@dangra
Copy link
Member

@dangra dangra commented Apr 21, 2015

You have my +1 to merge as long as tests passes in Travis-CI for python2 and python3

dangra added a commit that referenced this pull request Apr 22, 2015
[MRG] Item multi inheritance fix
@dangra dangra merged commit 571bf68 into scrapy:master Apr 22, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@coveralls
Copy link

@coveralls coveralls commented Dec 14, 2016

Coverage Status

Changes Unknown when pulling 7871acd on nramirezuy:item-multi_inherit into ** on scrapy:master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants