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

Handle typing issues hidden by follow_imports=skip #5805

Merged
merged 24 commits into from
Feb 24, 2023
Merged

Handle typing issues hidden by follow_imports=skip #5805

merged 24 commits into from
Feb 24, 2023

Conversation

wRAR
Copy link
Member

@wRAR wRAR commented Jan 25, 2023

This removes follow_imports = skip and fixes/ignores the issues that appeared.

There are many ignored issues regarding twisted.internet.reactor, it should often be possible to cast it into a suitable class/interface instead but I'm not sure how useful is that and also we still ignore interfaces (there is a WIP mypy plugin for them which I decided not to use). I also need to try ignoring twisted.internet.reactor via follow_imports = skip but Im not sure if it will work.

I've found a typing issue in w3lib (scrapy/w3lib#211) which we should fix and remove from here, and a possible error in the H2 code which we may need to check and rewrite.

@wRAR wRAR marked this pull request as draft January 25, 2023 20:03
@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #5805 (54ca682) into master (69d6894) will increase coverage by 0.00%.
The diff coverage is 96.55%.

❗ Current head 54ca682 differs from pull request most recent head 2970b35. Consider uploading reports for the commit 2970b35 to get more accurate results

@@           Coverage Diff           @@
##           master    #5805   +/-   ##
=======================================
  Coverage   88.94%   88.95%           
=======================================
  Files         162      162           
  Lines       11002    11011    +9     
  Branches     1798     1796    -2     
=======================================
+ Hits         9786     9795    +9     
- Misses        937      938    +1     
+ Partials      279      278    -1     
Impacted Files Coverage Δ
scrapy/core/http2/agent.py 96.42% <83.33%> (+0.04%) ⬆️
scrapy/core/http2/protocol.py 83.49% <85.71%> (+0.07%) ⬆️
scrapy/core/downloader/__init__.py 92.48% <100.00%> (ø)
scrapy/core/engine.py 84.19% <100.00%> (+0.17%) ⬆️
scrapy/core/http2/stream.py 91.90% <100.00%> (ø)
scrapy/core/scraper.py 84.61% <100.00%> (+0.08%) ⬆️
scrapy/core/spidermw.py 99.44% <100.00%> (ø)
scrapy/http/request/form.py 96.15% <100.00%> (+0.66%) ⬆️
scrapy/utils/defer.py 97.31% <100.00%> (ø)
scrapy/utils/python.py 90.96% <100.00%> (ø)
... and 1 more

@wRAR wRAR changed the title [WIP] Handle typing issues hidden by ignore-imports [WIP] Handle typing issues hidden by follow_imports=skip Jan 26, 2023
@wRAR wRAR changed the title [WIP] Handle typing issues hidden by follow_imports=skip Handle typing issues hidden by follow_imports=skip Feb 23, 2023
@wRAR wRAR marked this pull request as ready for review February 23, 2023 21:15
@@ -11,7 +11,7 @@ jobs:
- python-version: "3.11"
env:
TOXENV: pylint
- python-version: 3.7
- python-version: 3.8
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed because types-lxml is not available for 3.7.

3.7 EOL is in 4 months so I don't think this is a big problem.

@@ -83,7 +88,7 @@ def _remove_connection(self, errors: List[BaseException], key: Tuple) -> None:
pending_requests = self._pending_requests.pop(key, None)
while pending_requests:
d = pending_requests.popleft()
d.errback(errors)
d.errback(ResponseFailed(errors))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed not covered and I guess this is why it wasn't caught by tests.

warn_on_generator_with_return_value(spider, request.errback)
dfd = defer_fail(result)
dfd.addErrback(request.errback)
if request.errback:
dfd.addErrback(request.errback)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addErrback(None) is allowed and does nothing, but it's not documented (there is a comment saying "Default value used to be None" so maybe it was documented before but is now kept for backwards compativility) so I changed this.

@@ -50,7 +58,7 @@ def from_response(
response: TextResponse,
formname: Optional[str] = None,
formid: Optional[str] = None,
formnumber: Optional[int] = 0,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing None never worked.

scrapy/core/scraper.py Show resolved Hide resolved
@wRAR wRAR merged commit 8fbebfa into master Feb 24, 2023
@wRAR wRAR deleted the mypy-imports branch February 24, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants