-
Notifications
You must be signed in to change notification settings - Fork 122
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
#432 Use importlib.metadata.version
to get the version
#502
#432 Use importlib.metadata.version
to get the version
#502
Conversation
for more information, see https://pre-commit.ci
Some tests in Alternatives are to try to use |
See also: #491 |
I'm not sure how I feel about this whole"install towncrier along with your package" -- it kinda seems like a misfeature / attractive nuisance for the reasons we've just seen. :-/ I would argue for deprecation and adding ways to extract the version from the outside (pyproject.toml / extract using regexp from a file / run a command [e.g. hatch]). |
It would be much easier to use a |
It’s not that simple: hatch supports dynamic versions using plugins like I for one would claim that the user can run Don’t y’all feel too that these features are children of a bygone Python packaging era when things were simpler and more homogenous? |
I think that At the same time, I think that we should make simple things simple. So if the version is already defined as a simple text value inside the .toml file, I think that it makes sense to accept a PR in which the version is automatically extracted. We can document that this only works for limited use cases. I prefer to have all the common commands stored in file. If towncrier
or if someone already has the package installed, they can use something like
These options will not work for any case... but I don't think that we should reject a PR adding support for a specific use case. I expect that the majority of project will try to keep things simple, and have the version as a string inside the Just my feedback |
Hmm either way I'm sorry, I think the approach in this PR is the more correct one even though I disagree with the feature as a whole. It will need something like this and a conditional dependency Please consider this un-bikeshedded otherwise. |
So for this PR to be merged it needs:
I think there are already some tests generating With the automated tests, there is one big problem, if you start a python interpreter/process and while the process is running you install packages... the metadata of the new pacage might not be available. For my code sometimes I am calling
It works... if in the same process I call So just something to take into consideration when testing. |
I triggered the current test suite to see if there are any regression that might be introduced by this change. To me, this approach is harmeless ... if you don't have the package install it should work as before. Also, I don't know if we should worry about python 3.7 ... It will be EOL soon. |
How about we (read: you 😇) cut a release and drop 3.7 afterwards? (Pls someone merge my typo PR first tho 😅) |
Dropping Python 3.7 before this makes a lot of sense, though the backport of |
Well... I think that the overall feedback is that people are expecting from Using So the direction of this PR is good. But in order to merge this PR, we need all tests to be green. |
Oh yes, I just saw that. I meant that I could wait until the next release until 3.7 support is dropped as @hynek suggests, and then work on it |
src/towncrier/_project.py
Outdated
|
||
version = getattr(module, "__version__", None) | ||
try: | ||
version = metadata_version(f"{module}") |
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 str
of a module is not something you can pass to importlib.metadata.version
because you get a repr-like string:
>>> module = importlib.import_module("towncrier")
>>> print(f"{module}")
<module 'towncrier' from '/Users/.../site-packages/towncrier/__init__.py'>
which is probably just a small mistake here, but it reminds me that in fact version()
does not accept an Import Package name at all, but rather a Distribution Package. They're commonly the same but not necessarily so, to use some popular examples:
Import Package | Distribution Package |
---|---|
attr1 | attrs |
yaml | PyYAML |
As an example, let's say I'm the PyYAML developer, and I'm using towncrier with a __version__
variable via package = "yaml"
. If I want to drop that variable and specify my version in pyproject.toml
instead, then we can't pass yaml
to importlib.metadata.version
. We have to load a mapping from one to the other:
>>> import importlib.metadata
>>> importlib.metadata.packages_distributions()['yaml']
['PyYAML']
>>> importlib.metadata.version('PyYAML')
'6.0.1'
which in the context of get_version
would be something like:
dist_packages = distributions_packages()
try:
dist_package = dist_packages[package]
except KeyError:
# do fallback to package.__version__ here
else:
if len(dist_package) > 1:
raise Exception("I found more than one package")
version = metadata_version(dist_package[0])
So that's how we'd make towncrier use importlib.metadata in a backwards-compatible way (user doesn't have to change their tool.towncrier
config).
It's a little unfortunate, though, that I have to do this:
[project]
name = "PyYAML"
version = "1.2.3"
[tool.towncrier]
package = "yaml"
The version associated with the distribution package is right there, but then I have to tell towncrier one of the import packages I have so that it can look up the name of my distribution package.
PEP 621 says that the project.name
is required to be statically defined, which means towncrier could try to read project.name
by default if package
is not defined and --version
is not passed.
Notably Poetry doesn't yet support PEP 621 but the package
would still be there for Poetry users.
Footnotes
-
attrs also has its newer
attrs
namespace of course :-) ↩
I realise that this PR has been lost in the hallows of my GitHub account... and it has taken almost an year for me to get back to this, following the notification for the received review, thanks! 🙂 I will shall look to rebase this branch soon and finish this up, because it looks like other PRs are now depending on this (such as #491). |
So something like my last commit in this PR, @RazerM ? |
I think the message in the exception raised needs to be updated as well for the test to pass |
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.
It looks good. Thanks.
Only minor comments.
There might be things that can be improved. but I think that we can do that in a separate PR.
Thanks again.
pyproject.toml
Outdated
@@ -31,6 +31,7 @@ requires-python = ">=3.8" | |||
dependencies = [ | |||
"click", | |||
"importlib-resources>=5; python_version<'3.10'", | |||
"importlib-metadata==4.6.*; python_version<'3.10'", |
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 that this needs a comment to explain why we are pinning on 4.6 version.
Why not use the latest version ?
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.
A better pin would be >=4.6 I guess (this is the version that had parity with Python 3.10).
Description
This PR updates the
get_version()
method insrc/towncrier/_project.py
to useimportlib.metadata.version()
[Fixes #432]Checklist
src/towncrier/newsfragments/
. Describe yourchange and include important information. Your change will be included in the public release notes.
docs/tutorial.rst
is still up-to-date.docs/cli.rst
reflects those changes.docs/configuration.rst
reflects those changes.