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

[get_retry_request] Adding ability to set log level #5625

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

further-reading
Copy link
Contributor

@further-reading further-reading commented Sep 13, 2022

Fixes #5297, fixes #4622

@codecov
Copy link

codecov bot commented Sep 13, 2022

Codecov Report

Merging #5625 (dda9ce4) into master (3cf97e4) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5625      +/-   ##
==========================================
+ Coverage   88.66%   88.69%   +0.03%     
==========================================
  Files         162      162              
  Lines       10962    10969       +7     
  Branches     1894     1796      -98     
==========================================
+ Hits         9719     9729      +10     
+ Misses        963      960       -3     
  Partials      280      280              
Impacted Files Coverage Δ
scrapy/downloadermiddlewares/retry.py 100.00% <100.00%> (ø)
scrapy/http/response/text.py 100.00% <100.00%> (ø)
scrapy/settings/default_settings.py 98.77% <100.00%> (+<0.01%) ⬆️
scrapy/utils/python.py 88.46% <0.00%> (-0.13%) ⬇️
scrapy/core/downloader/tls.py 97.05% <0.00%> (-0.09%) ⬇️
scrapy/spiders/crawl.py 94.31% <0.00%> (+0.34%) ⬆️
scrapy/core/downloader/handlers/http11.py 94.09% <0.00%> (+0.96%) ⬆️

@wRAR
Copy link
Member

wRAR commented Sep 14, 2022

How would one use this? By subclassing the middleware?

@further-reading
Copy link
Contributor Author

How would one use this? By subclassing the middleware?

The get_retry_request function is was added last year so one could trigger retries in a callback. https://docs.scrapy.org/en/latest/news.html#scrapy-2-5-0-2021-04-06

The purpose of this issue is to add the ability to set log level when using it in a callback. There's a seperate issue #4622 for extending the middleware to then use this feature of get_retry_request, which I'm also going to work on.

@further-reading
Copy link
Contributor Author

I've updated the middleware so this will fix #4622 as well.

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

The code looks good to me, beyond my minor feedback.

Next we would need tests to cover the default function value, the ability to override through the function, and same for the setting. And a short documentation entry for the new setting.

scrapy/downloadermiddlewares/retry.py Outdated Show resolved Hide resolved
@wRAR
Copy link
Member

wRAR commented Oct 14, 2022

@Gallaecio I see you previously wanted tests and docs but you've approved this now, so what should I do? :)

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

I forgot about that 🤦

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.

get_retry_request: allow changing the log level of the gave up message Retry logs an error when giving up
3 participants