-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
Cleanup settings._DictProxy and scrapy.telnet #5730
Conversation
DEPRECATION_RULES = [ | ||
('scrapy.telnet.', 'scrapy.extensions.telnet.'), | ||
] | ||
DEPRECATION_RULES = [] | ||
|
||
|
||
def update_classpath(path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a possibility of the DEPRECATION_RULES
list being repopulated in the future due to further deprecation of packages then I would recommend to keep the update_classpath
method otherwise similar logic will need to be re-written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be not the case since @deprecated
decorator already have this functionality:
scrapy/scrapy/utils/decorators.py
Line 9 in 45c2bd7
def deprecated(use_instead=None): |
But let's see what Scrapy team has to say about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update_classpath
is used in the code that parses object dicts such as DOWNLOADER_MIDDLEWARES
:
Line 15 in 45c2bd7
def build_component_list(compdict, custom=None, convert=update_classpath): |
I think you can't use @deprecated
for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but the logic depends all on DEPRECATION_RULES
variable right?
for prefix, replacement in DEPRECATION_RULES:
if isinstance(path, str) and path.startswith(prefix):
new_path = path.replace(prefix, replacement, 1)
warnings.warn(f"`{path}` class is deprecated, use `{new_path}` instead",
ScrapyDeprecationWarning)
return new_path
return path
If nothing is provided it will just return what was originally send by parameter right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not against removing it.
However, then we would need to restore it if we deprecate a component path in the future, and chances are whoever does it will not realize there was a previous implementation that can be restored, and will waste time coming up with a new implementation, possibly worse than this one.
So keeping this code around, even if currently unused, may be the lesser evil.
Codecov Report
@@ Coverage Diff @@
## master #5730 +/- ##
==========================================
+ Coverage 88.73% 88.78% +0.04%
==========================================
Files 162 162
Lines 10964 10949 -15
Branches 1799 1799
==========================================
- Hits 9729 9721 -8
+ Misses 954 946 -8
- Partials 281 282 +1
|
@wRAR, is this PR ready to be merged? |
@hanzallah we need two approvals from the core contributors to merge a PR. |
Great, thanks! |
This change removes the
_DictProxy
class fromscrapy/settings/__init__.py
since it is no longer used. It also removesscrapy.telnet
from theDEPRECATION_RULES
as part of the cleanup. It resolves #5729, resolves #5725.