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

Improve typing for Spider.parse(). #6274

Merged
merged 4 commits into from May 13, 2024
Merged

Conversation

wRAR
Copy link
Member

@wRAR wRAR commented Mar 6, 2024

Based on https://stackoverflow.com/questions/77160087/correct-typing-of-a-method-without-a-fixed-signature

Should remove most of the mypy errors for parse() in user spiders (if they use older mypy Spider.parse() will appear untyped so no override error either). Doesn't help with scrapy-poet's callbacks that narrow down Response to DummyResponse: scrapinghub/scrapy-poet#179

This should probably have typing tests, I'll do that later.

@wRAR wRAR added the typing label Mar 6, 2024
@wRAR wRAR added this to the Scrapy 2.12 milestone Mar 6, 2024
@wRAR wRAR marked this pull request as draft March 6, 2024 19:01
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 37.50000% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 88.79%. Comparing base (563ecbe) to head (84056f4).
Report is 16 commits behind head on master.

❗ Current head 84056f4 differs from pull request most recent head 050aef3. Consider uploading reports for the commit 050aef3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6274      +/-   ##
==========================================
+ Coverage   84.88%   88.79%   +3.90%     
==========================================
  Files         161      161              
  Lines       11927    11867      -60     
  Branches     1810     1919     +109     
==========================================
+ Hits        10124    10537     +413     
+ Misses       1523      982     -541     
- Partials      280      348      +68     
Files Coverage Δ
scrapy/commands/bench.py 100.00% <100.00%> (+4.44%) ⬆️
scrapy/spiders/__init__.py 88.23% <28.57%> (-11.77%) ⬇️

... and 95 files with indirect coverage changes


from scrapy.crawler import Crawler
from scrapy.settings import BaseSettings

CallbackT = Callable[Concatenate[Response, ...], Any]
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 could be useful in other places as it's a general type for a spider callback. I didn't think about that further though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually we should use it for Request.callback

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like we can't, probably because of wrong covariance when assigning a specific callback to a request, I need to investigate this separately.

@wRAR wRAR marked this pull request as ready for review May 11, 2024 17:36
@Gallaecio Gallaecio merged commit 93f0628 into scrapy:master May 13, 2024
53 checks passed
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.

None yet

2 participants