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

Use packages to extract version information #258

Closed
wants to merge 1 commit into from

Conversation

jagerber48
Copy link
Contributor

  • Closes # (insert issue number)

  • Executed pre-commit run --all-files with no errors

  • The change is fully covered by automated unit tests

  • Documented in docs/ as appropriate

  • Added an entry to the CHANGES file

  • Use importlib.metadata to extract __version__ and packaging.version to extract __version_info__ tuple. In my opinion, this is an improvement over importing from the dynamically generated version.py.

The goal here is to "single source" the version number. In this configuration with setuptools_scm the single source is the git tag. Package metadata is populated at build time and version information is extracted from there.

Copy link

codecov bot commented Jul 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.33%. Comparing base (4a470e4) to head (f8cbef5).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #258   +/-   ##
=======================================
  Coverage   96.33%   96.33%           
=======================================
  Files          15       15           
  Lines        1909     1911    +2     
=======================================
+ Hits         1839     1841    +2     
  Misses         70       70           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@newville
Copy link
Member

@jagerber48 What problem does this solve? If this is desirable, you would probably want to take out the creation of version.py in pyproject.toml. But, seeing as I changed from "edit __version__ by hand in __init__.py" to "use setuptools_scm to write to version.py and import from there" just a couple of weeks ago, and did not add other imports in __init__.py, you might guess that I am not 100% in favor here.;)

It's sort of exhausting trying to keep up. I am going to have to take a break from this project and will not be able to participate in discussions, reviews, or development of uncertainties for the next two weeks.

@wshanks
Copy link
Collaborator

wshanks commented Jul 21, 2024

I also don't have much time for the next couple of weeks still. I wanted to note though that packaging is not a standard library module and so should be added as a dependency in pyproject.toml. I don't see it getting installed in the log -- it must be installed by the setup-python GitHub Action.

Also, I think we agreed that we would only support Python 3.8+, but I just want to note that this might be the first change that actually breaks Python 3.7 (since importlib.metadata was added in 3.8).

@reneeotten
Copy link
Collaborator

the "single source of the version" should be the git tag. I totally agree with @newville there is nothing gained from this, it just unnecessarily add random dependencies.

@jagerber48
Copy link
Contributor Author

Ok, pretty negative response to this one. I'll just go ahead and close it. Thanks for the responses nonetheless. For what it's worth, I'll give my responses to say my peace. First off, I had my bout of about a month or two when I wasn't able to keep up here and a lot seemed to have happened. I'm still catching up on things. Sincere apologies for rehashing anything.

@newville I think the move to using setuptools_scm to extract the package version from the git tag rather than having devs update the version manually in __init__.py and possibly other places was a great move. If I had done it then, yes, I would have left off the [tool.setuptools_scm] section with the write_to flag. If we insist on __version__ being importable (you convinced me this is a good idea with your survey of other packages) then I would have set __version__ = importlib.metadata.version('uncertainties') like done in this PR. I don't see the advantages of version.py over this approach. version.py just creates more configuration, it requires the extra section in pyproject.toml and it adds something to be kept track of in .gitignore. this article demonstrates both of these approaches.

That said, using the importlib.metadata approach for __version__ doesn't leave me with a good strategy for extracting __version_info__. I'm not familiar with __version_info__. It seemed like a bad idea to spin my own version parsing in __init__.py so I looked for an existing solution. What is in this PR is what I hit on first. I didn't realize that packaging is not stdlib, otherwise I wouldn't have added it. @wshanks thanks for kindly pointing this out to me. If it was me I would have asked if we could leave off __version_info__. I think it used to be used in uncertainties before we made changes, when a lot of version number checks were going on to ensure compatibility. But those are all gone now. I wouldn't mind making the breaking changes of pulling __version_info__.

@wshanks that would be interesting if this is the first change to break 3.7. Thanks for pointing that out also. If I get a chance I'll test the current code on 3.7.

@reneeotten Yes, I agree git tag should be the single source. This PR still has git tag as the single source. I argue it holds better to the "single source" spirit than the current version of the code because it writes the version information to one less place. As far as I can tell, the current version of code writes the version number to go PKG-INFO and version.py at build time. This PR would just write it to PKG-INFO. I didn't realize that I was adding a packaging dependency and a python version dependency here.

@jagerber48 jagerber48 closed this Jul 22, 2024
@jagerber48 jagerber48 deleted the better_version_handling branch July 22, 2024 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants