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

Typing for scrapy/utils #5925

Merged
merged 19 commits into from
Jun 24, 2023
Merged

Typing for scrapy/utils #5925

merged 19 commits into from
Jun 24, 2023

Conversation

wRAR
Copy link
Member

@wRAR wRAR commented May 7, 2023

Another batch of improved typing, mostly covering scrapy.utils.* (not everything there was processed yet but I think it's already quite large).

I have one question for @elacuesta: #3869 added scrapy.utils.datatypes.LocalWeakReferencedCache which returns None if "key is not weak-referenceable", and because of how scrapy.utils.misc.is_generator_with_return_value() uses it, that function can return None in addition to an expected bool. It even uses a pattern of putting a value into a dict and getting it back. So I wonder if None there has the same value as False (as the result of is_generator_with_return_value() is only checked with a simple if) and if it's fine to cast it to bool like I do in this PR.

@wRAR wRAR added the typing label May 7, 2023
@codecov
Copy link

codecov bot commented May 7, 2023

Codecov Report

Merging #5925 (2f256d6) into master (85f5122) will increase coverage by 0.29%.
The diff coverage is 94.52%.

❗ Current head 2f256d6 differs from pull request most recent head 712ee98. Consider uploading reports for the commit 712ee98 to get more accurate results

@@            Coverage Diff             @@
##           master    #5925      +/-   ##
==========================================
+ Coverage   88.89%   89.19%   +0.29%     
==========================================
  Files         162      162              
  Lines       11254    11287      +33     
  Branches     1827     1833       +6     
==========================================
+ Hits        10004    10067      +63     
+ Misses        965      928      -37     
- Partials      285      292       +7     
Impacted Files Coverage Δ
scrapy/utils/engine.py 77.27% <71.42%> (-6.94%) ⬇️
scrapy/utils/request.py 96.09% <77.77%> (-1.53%) ⬇️
scrapy/utils/display.py 93.10% <85.71%> (+0.24%) ⬆️
scrapy/utils/url.py 96.36% <86.66%> (-3.64%) ⬇️
scrapy/utils/decorators.py 76.66% <88.88%> (+1.66%) ⬆️
scrapy/utils/deprecate.py 93.05% <89.47%> (-2.40%) ⬇️
scrapy/utils/python.py 88.75% <95.83%> (ø)
scrapy/crawler.py 87.37% <100.00%> (+0.06%) ⬆️
scrapy/resolver.py 92.30% <100.00%> (+0.09%) ⬆️
scrapy/spiderloader.py 100.00% <100.00%> (ø)
... and 10 more

... and 2 files with indirect coverage changes

@wRAR
Copy link
Member Author

wRAR commented May 9, 2023

collections.OrderedDict and weakref.WeakKeyDictionary are not generic in 3.7 and 3.8, the former can be replaced with typing.OrderedDict while the latter I think doesn't have that option, so I think I'll just remove specialization from it.

@wRAR wRAR merged commit 5105742 into scrapy:master Jun 24, 2023
@wRAR wRAR deleted the typing-utils branch June 24, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants