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

Obsolete REDIRECT_MAX_METAREFRESH_DELAY, LOG_UNSERIALIZABLE_REQUESTS #4385

Merged
merged 1 commit into from
Mar 4, 2020

Conversation

nyov
Copy link
Contributor

@nyov nyov commented Feb 28, 2020

Obsolete REDIRECT_MAX_METAREFRESH_DELAY which has been deprecated since Scrapy 0.18
defc4f8
and LOG_UNSERIALIZABLE_REQUESTS which has been deprecated since Scrapy 1.2.0
472a8a4

@nyov nyov changed the title Obsolete REDIRECT_MAX_METAREFRESH_DELAY Obsolete REDIRECT_MAX_METAREFRESH_DELAY, LOG_UNSERIALIZABLE_REQUESTS Feb 28, 2020
@codecov
Copy link

codecov bot commented Feb 28, 2020

Codecov Report

Merging #4385 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #4385   +/-   ##
=======================================
  Coverage   84.16%   84.16%           
=======================================
  Files         166      166           
  Lines        9970     9970           
  Branches     1483     1483           
=======================================
  Hits         8391     8391           
  Misses       1324     1324           
  Partials      255      255
Impacted Files Coverage Δ
scrapy/settings/deprecated.py 50% <ø> (ø) ⬆️
scrapy/core/scheduler.py 90% <100%> (ø) ⬆️
scrapy/downloadermiddlewares/redirect.py 97.01% <100%> (ø) ⬆️

@Gallaecio
Copy link
Member

Gallaecio commented Feb 28, 2020

Could you please also remove the corresponding entries from scrapy/settings/deprecated.py? (maybe you will find there some other deprecated settings that are also old enough to stop supporting)

@nyov
Copy link
Contributor Author

nyov commented Feb 28, 2020

I was thinking of entirely removing all the entries there ;)
All of them are entirely obsolete (not just deprecated) and no longer found in the source.

But then I had the thought they they may also be meant to remind people, who still have them in legacy projects, where the setting doesn't do anything anymore (without people realising that, possibly).

Do you think we should clean them up, instead? I wouldn't mind that either.

@Gallaecio
Copy link
Member

Gallaecio commented Feb 28, 2020

As far as I known, the deprecation messages are only meant to last as much as the code that supports the deprecated functionality:

  1. The functionality is implemented.
  2. The functionality is deprecated, it continues to work but a warning is issued.
  3. The functionality and the warning are removed, clean code!

So, at least for those two deprecated variables you are removing, I think we should remove them from that file as well.

As for the other ones, it really depends on how long those warnings have been there for. Based on #4356 I would say there is an agreement that deprecations 3+ years old are OK to remove (along with code to support old behavior when there is such code). But it can be done in separate PRs.

@nyov
Copy link
Contributor Author

nyov commented Feb 28, 2020

Ok, added. Err, removed.
I'll squash once review agrees that these should be removed here.

(In that event, I'll have another PR which kills the rest of 'em, since they're even older. :goberserk: ;)

@nyov
Copy link
Contributor Author

nyov commented Mar 3, 2020

Squashed

Obsolete REDIRECT_MAX_METAREFRESH_DELAY
  which has been deprecated since Scrapy 0.18

Obsolete LOG_UNSERIALIZABLE_REQUESTS
  which has been deprecated since Scrapy 1.2.0
  and is replaced by SCHEDULER_DEBUG
@kmike
Copy link
Member

kmike commented Mar 4, 2020

Thanks @nyov!

@kmike kmike merged commit ff8e826 into scrapy:master Mar 4, 2020
@nyov nyov deleted the obsolete branch March 4, 2020 20:13
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.

4 participants