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

Provide complete API documentation coverage of scrapy.item #3999

Merged
merged 1 commit into from Sep 25, 2019

Conversation

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Sep 3, 2019

As part of these changes I was going to document scrapy.item.DictItem. However, trying to understand its purpose, I came to the conclusion that it is a leftover that no longer has a purpose, so in this set of changes I’m deprecating it.

I believe it was introduced in decbdc5 so that DjangoItem could have everything that Item has but using a different metaclass. However, Item seems to be currently used in scrapy-djangoitem instead.

I am not 100% sure about scrapy.item.DictItem no longer having a purpose, though. So please, look into it for yourselves before approving these changes.

@elacuesta
Copy link
Member

@elacuesta elacuesta commented Sep 3, 2019

Sounds good to me, for the sake of simplicity and having less classes that do basically the same. However I think the Item class definition should be modified not to inherit from DictItem (Github won't let me comment in the appropriate line because it's not modified in this PR).
Perhaps the names could be swapped, so the Item class doesn't lose all the methods it currently inherits from DictItem.

@Gallaecio
Copy link
Member Author

@Gallaecio Gallaecio commented Sep 5, 2019

I think the Item class definition should be modified not to inherit from DictItem

I’m not sure we need to change the inheritance tree at this point. Although unlikely, it’s possible that changing the inheritance breaks code silently (e.g. isinstance(this, DictItem) would stop evaluating to True for Item).

If instead we keep the inheritance tree as is, and simply remove DictItem in a future version (moving all its code to Item then), such code will fail as well, but loudly with an error upon importing DictItem.

@elacuesta
Copy link
Member

@elacuesta elacuesta commented Sep 5, 2019

Forget what I said, I got confused. cls is DictItem is True only when the object being created is in fact a DictItem, not when creating from a subclass like Item.

scrapy/item.py Outdated Show resolved Hide resolved
@Gallaecio Gallaecio force-pushed the documentation-coverage branch from 5e2f333 to 2239c6a Sep 16, 2019
@codecov
Copy link

@codecov codecov bot commented Sep 16, 2019

Codecov Report

Merging #3999 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3999      +/-   ##
==========================================
+ Coverage   85.67%   85.68%   +<.01%     
==========================================
  Files         165      165              
  Lines        9726     9732       +6     
  Branches     1461     1462       +1     
==========================================
+ Hits         8333     8339       +6     
  Misses       1136     1136              
  Partials      257      257
Impacted Files Coverage Δ
scrapy/item.py 98.66% <100%> (+0.11%) ⬆️

@Gallaecio
Copy link
Member Author

@Gallaecio Gallaecio commented Sep 24, 2019

Should I write a test to cover the code changes here?

@kmike
Copy link
Member

@kmike kmike commented Sep 24, 2019

@Gallaecio yes, could you please add a test for it?

@Gallaecio Gallaecio force-pushed the documentation-coverage branch from 2239c6a to 1236e9e Sep 25, 2019
@Gallaecio
Copy link
Member Author

@Gallaecio Gallaecio commented Sep 25, 2019

Tests added, and implementation improved to also warn about inheriting from DictItem.

@kmike kmike merged commit 31c631f into scrapy:master Sep 25, 2019
2 checks passed
@kmike
Copy link
Member

@kmike kmike commented Sep 25, 2019

Looks good, thanks @Gallaecio!

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

3 participants