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

item fields defined on fields class attribute fix #1228

Merged
merged 1 commit into from May 15, 2015

Conversation

@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented May 13, 2015

reported on: #1217

@nramirezuy nramirezuy force-pushed the nramirezuy:item_fields_defined_on_fields branch from c85ae02 to ce2e2be May 13, 2015
@dangra
Copy link
Member

@dangra dangra commented May 13, 2015

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
Copy link
Contributor Author

@nramirezuy nramirezuy commented May 13, 2015

It was working before and was fair easy to fix it.
I never defined an item that way; the only functionality I can think of is consistency with another side effect 👅

>>> 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
Copy link
Member

@dangra dangra commented May 13, 2015

Ok, what do you think about adding a very simple test case without inheritance ?

@nramirezuy nramirezuy force-pushed the nramirezuy:item_fields_defined_on_fields branch from bb20e13 to a220bf7 May 13, 2015
@nramirezuy
Copy link
Contributor Author

@nramirezuy nramirezuy commented May 13, 2015

@dangra That works?

@dangra
Copy link
Member

@dangra dangra commented May 13, 2015

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().

@nramirezuy nramirezuy force-pushed the nramirezuy:item_fields_defined_on_fields branch from a220bf7 to 773ea5a May 13, 2015
@nramirezuy
Copy link
Contributor Author

@nramirezuy nramirezuy commented May 13, 2015

done

@dangra
Copy link
Member

@dangra dangra commented May 13, 2015

Do we need documentation updates?

anyone want to comment before merging? /cc @curita @kmike

dangra added a commit that referenced this pull request May 15, 2015
item fields defined on fields class attribute fix
@dangra dangra merged commit 5e5e44e into scrapy:master May 15, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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

2 participants
You can’t perform that action at this time.