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

bpo-12915: Add pkgutil.resolve_name #18310

Merged
merged 6 commits into from Feb 14, 2020
Merged

bpo-12915: Add pkgutil.resolve_name #18310

merged 6 commits into from Feb 14, 2020

Conversation

vsajip
Copy link
Member

@vsajip vsajip commented Feb 2, 2020

@vsajip vsajip added the type-feature A feature request or enhancement label Feb 2, 2020
@AstraLuma
Copy link

AstraLuma commented Feb 13, 2020

Some design questions:

  • Why is the dots-only form "deprecated'?
  • Why does the dots-only form not support traversing more than one deep? ("mypkg.mymod.MyClass.my_constructor" will fail to resolve)

I would also comment that requiring the name parts to be valid Python names isn't technically required and adds complexity, although I expect situations where it's a hindrance are few and far between.

@vsajip
Copy link
Member Author

vsajip commented Feb 13, 2020

Why is the dots-only form "deprecated"?

The word "deprecate" isn't used anywhere. The reason for preferring the new form is given in the documentation.

Why does the dots-only form not support traversing more than one deep? ("mypkg.mymod.MyClass.my_constructor" will fail to resolve)

On what do you base this assertion? For example, logging.handlers.SysLogHandler.LOG_ALERT resolves correctly. I'll update the tests to confirm this.

I would also comment that requiring the name parts to be valid Python names isn't technically required and adds complexity

I don't find the current implementation too complex, and generally attributes are Python identifiers (though of course one might be able to get around that by overriding __getattr__ / __getattribute__) it wouldn't be considered good practice to have attributes which don't meet this constraint.

Copy link
Member

@merwok merwok left a comment

Choose a reason for hiding this comment

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

Thanks for taking up this patch!

Some suggestions for the docs and unit tests.

Doc/library/pkgutil.rst Outdated Show resolved Hide resolved
Doc/library/pkgutil.rst Outdated Show resolved Hide resolved
Lib/pkgutil.py Outdated Show resolved Hide resolved
Lib/pkgutil.py Outdated Show resolved Hide resolved
Lib/test/test_pkgutil.py Outdated Show resolved Hide resolved
Lib/test/test_pkgutil.py Outdated Show resolved Hide resolved
Lib/test/test_pkgutil.py Outdated Show resolved Hide resolved
@merwok
Copy link
Member

merwok commented Feb 13, 2020

I would also comment that requiring the name parts to be valid Python names isn't technically required and adds complexity,

For comparison, here are other implementations:

I think that Vinay’s solution using a regex has a nice balance of checking validity without the performance hit of calling for example part.isidentifier().

@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #18310 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18310      +/-   ##
==========================================
- Coverage   82.20%   82.12%   -0.09%     
==========================================
  Files        1957     1955       -2     
  Lines      588968   584020    -4948     
  Branches    44401    44456      +55     
==========================================
- Hits       484160   479607    -4553     
+ Misses      95153    94767     -386     
+ Partials     9655     9646       -9     
Impacted Files Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py 30.00% <0.00%> (-65.00%) ⬇️
Lib/distutils/command/bdist_rpm.py 7.63% <0.00%> (-56.88%) ⬇️
Lib/test/test_urllib2net.py 76.92% <0.00%> (-13.85%) ⬇️
Lib/test/test_smtpnet.py 78.57% <0.00%> (-12.86%) ⬇️
Lib/ftplib.py 63.85% <0.00%> (-6.06%) ⬇️
Lib/test/test_ftplib.py 87.11% <0.00%> (-4.72%) ⬇️
Lib/dbm/__init__.py 66.66% <0.00%> (-4.45%) ⬇️
Tools/scripts/db2pickle.py 17.82% <0.00%> (-3.97%) ⬇️
Lib/test/test_socket.py 71.94% <0.00%> (-3.87%) ⬇️
Tools/scripts/pickle2db.py 16.98% <0.00%> (-3.78%) ⬇️
... and 369 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78c7183...0c922dd. Read the comment docs.

@merwok
Copy link
Member

merwok commented Feb 14, 2020

I notice that module. is not supported (rightly!), but module: is; is that something officially supported by setuptools’ entry points / other resolve implementations?

Will you update pydoc and other modules to use this new function in a separate PR?

@vsajip
Copy link
Member Author

vsajip commented Feb 14, 2020

module: is; is that something officially supported by setuptools’ entry points / other resolve implementations?

Not as far as I know, because it doesn't especially make sense for entry points. It seemed reasonable to support this use case, and no reason not to allow it (although not useful for entry points, it could be useful in other contexts).

Will you update pydoc and other modules to use this new function in a separate PR?

Yes, and possibly using more than one PR.

Copy link
Member

@merwok merwok left a comment

Choose a reason for hiding this comment

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

Not sure about allowing module:, but in general +1!

Lib/pkgutil.py Outdated
mod = importlib.import_module(modname)
while parts:
p = parts[0]
s = '%s.%s' % (modname, p)
Copy link
Member

Choose a reason for hiding this comment

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

Minor: % formatting here and on line 679.

@merwok merwok changed the title bpo-12915: Added pkgutil.resolve_name(). bpo-12915: Add pkgutil.resolve_name Feb 14, 2020
@vsajip vsajip merged commit 1ed6161 into python:master Feb 14, 2020
@vsajip vsajip deleted the fix-12915 branch February 14, 2020 22:02
sthagen added a commit to sthagen/python-cpython that referenced this pull request Feb 15, 2020
bpo-12915: Add pkgutil.resolve_name (pythonGH-18310)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants