-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
bpo-29576: Improve some deprecations in the importlib #32
Conversation
Lib/importlib/abc.py
Outdated
""" | ||
warnings.warn("MetaPathFinder.find_module is deprecated since Python " | ||
"3.4 in favor of MetaPatFinder.find_spec " |
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.
typo here s/MetaPatFinder/MetaPathFinder/
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.
Oops indeed, thanks.
8cc3e57
to
58d8d3c
Compare
Codecov Report
@@ Coverage Diff @@
## master #32 +/- ##
==========================================
- Coverage 82.37% 82.36% -0.01%
==========================================
Files 1427 1427
Lines 350948 350951 +3
==========================================
- Hits 289093 289062 -31
- Misses 61855 61889 +34 Continue to review full report at Codecov.
|
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.
Could you add tests to make sure the warnings are being raised?
Lib/importlib/abc.py
Outdated
@@ -97,7 +106,13 @@ def find_loader(self, fullname): | |||
This method is deprecated in favor of finder.find_spec(). If find_spec() | |||
is provided than backwards-compatible functionality is provided. | |||
|
|||
Deprecated since Python 3.4 in favor of find_spec (available since 3.4) |
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.
This line is somewhat redundant since the deprecation is mentioned in the previous paragraph. Maybe just add the version it was deprecated in above?
Lib/importlib/abc.py
Outdated
@@ -61,7 +64,13 @@ def find_module(self, fullname, path): | |||
exists then backwards-compatible functionality is provided for this | |||
method. | |||
|
|||
Deprecated since Python 3.4 in favor of find_spec (available since 3.4) |
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.
The deprecation is mentioned in the above paragraph, so if you would like to add the version info just toss in up there.
Lib/importlib/abc.py
Outdated
""" | ||
warnings.warn("MetaPathFinder.find_module is deprecated since Python " | ||
"3.4 in favor of MetaPathFinder.find_spec " |
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.
Please add ()
after find_spec
.
Lib/importlib/abc.py
Outdated
""" | ||
warnings.warn("PathEntryFinder.find_loader is deprecated since Python " | ||
"3.4 in favor of PathEntryFinder.find_spec " |
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.
Please add()
as appropriate.
Add the python version since the functionality is deprecated, and raise a couple of deprecation warnings in a few places. Theses functions are marked as deprecated in the documentation, but especially in existing codebase, programmers tends to not re-check whether functions are deprecated. So trigger the warning when possible. It's also more probable that a developer will drop deprecated functionality if we immediately give them information about replacement API, and not have them to go find it in the documentation. Include deprecation information in DocString as well as many tools pull documentation from there and not from docs.python.org. Add test making sure `find_loader()` and `find_module()` Both emit a deprecation warning.
58d8d3c
to
51e8898
Compare
All comments should be addressed. |
Thanks! Please add entries in Misc/NEWS and one in What's New in the "deprecated" section and this will be ready to be merged! |
Thanks! |
Thanks ! |
Add the python version since the functionality is deprecated,
and raise a couple of deprecation warnings in a few places.
Theses functions are marked as deprecated in the documentation, but
especially in existing codebase, programmers tends to not re-check
whether functions are deprecated. So trigger the warning when possible.
It's also more probable that a developer will drop deprecated
functionality if we immediately give them information about
replacement API, and not have them to go find it in the documentation.
Include deprecation information in DocString as well, as many tools pull
documentation from there and not from docs.python.org.