Skip to content

changed version_option default package from pkg_resources to importli…#1582

Merged
davidism merged 3 commits into
pallets:masterfrom
teymour-aldridge:version-check
Jun 24, 2020
Merged

changed version_option default package from pkg_resources to importli…#1582
davidism merged 3 commits into
pallets:masterfrom
teymour-aldridge:version-check

Conversation

@0x1za

@0x1za 0x1za commented Jun 10, 2020

Copy link
Copy Markdown
Contributor

I believe this should close issue #1488

@0x1za

0x1za commented Jun 10, 2020

Copy link
Copy Markdown
Contributor Author

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

Comment thread src/click/decorators.py Outdated
Comment thread requirements/dev.txt Outdated
Comment thread src/click/decorators.py Outdated

@0x1za 0x1za left a comment

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

davidism commented Jun 24, 2020

Copy link
Copy Markdown
Member

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.

2 participants