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

changed version_option default package from pkg_resources to importli… #1582

Merged
merged 3 commits into from Jun 24, 2020

Conversation

0x1za
Copy link
Contributor

@0x1za 0x1za commented Jun 10, 2020

I believe this should close issue #1488

@0x1za
Copy link
Contributor Author

0x1za commented Jun 10, 2020

I removed a couple of no longer needed variables and imports.

src/click/decorators.py Outdated Show resolved Hide resolved
requirements/dev.txt Outdated Show resolved Hide resolved
src/click/decorators.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@0x1za 0x1za left a comment

Choose a reason for hiding this comment

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

LGTM

0x1za and others added 2 commits June 24, 2020 14:03
option() returns a decorator, no need for inner decorator function
optional package_name parameter to skip stack frame detection
package_name is available in message format
use nonlocal keyword for modifying args from callback
only detect version if package name was detected
show error if importlib_metadata needs to be installed
show error if detected package name isn't an installed package name
show detected package name in error when version wasn't detected
rewrite docs
@davidism
Copy link
Member

Squashed the original commits, added a changelog, added a change to requirements/tests.in that didn't get committed before. Then, while reviewing this, I realized that the entire function could use a refactor and new docs, so I did that in a separate commit.

  • option() returns a decorator, so there's no point in wrapping it in another decorator internally.
  • Version detection relies on being able to detect the module defining the CLI by examining the stack frames. Added the package_name parameter to skip or override detection if it fails. Also make package available in the message format, since it can be different than prog.
  • Use inspect.currentframe() for stack inspection instead of a private method. Gets the top-level package name instead of the full dotted __name__.
  • Use Python 3's nonlocal keyword for modifying args from the callback.
  • Only try to detect the version if a package name was given or detected.
  • Show a specific error if importlib_metadata needs to be installed.
  • Show an error if metadata.version() can't find the package. This can happen if the package name doesn't match the PyPI name.
  • Show the detected package name in the error message when the version wasn't detected, to make it clearer what Click was trying.

Also noticed that the change you made changed how the version was being detected, from looking at entry points to looking at the package name directly. It looks like the values returned by importlib.metadata.entry_points() aren't linked to the distributions that own them, so there's not really a nice way to use the previous implementation. Writing some notes below so the difference is clear later.


The previous implementation tried to find the program name among the console_scripts entry points, then if the module that entry pointed at matched the stack frame detected module, it took the version of the distribution that created the entry. The new implementation skips looking at entry points and assumes the package name is the same as the installed name. While that's not strictly true, it's definitely a good and common practice. The previous implementation wasn't strictly correct either, as there was no guarantee that the module that created the version option was also the one pointed to by the entry point.

Co-authored-by: Claudio Jolowicz <claudio.jolowicz@cyren.com>
@davidism
Copy link
Member

davidism commented Jun 24, 2020

Incorporated #1531 for better version detection when using python -m package. Also noticed that the original code wasn't breaking the reference cycle on frame, so fixed that.

@davidism davidism added this to the 8.0.0 milestone Jun 24, 2020
@davidism davidism merged commit 77ca520 into pallets:master Jun 24, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants