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

Replace deprecated package pkg_resources #36

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Replace deprecated package pkg_resources #36

merged 3 commits into from
Oct 10, 2024

Conversation

jeffjennings
Copy link
Contributor

@jeffjennings jeffjennings commented Oct 9, 2024

pkg_resources is deprecated; this PR replaces its single use in the code with importlib_metadata as recommended in the first link.

Closes #37

@jeffjennings jeffjennings requested review from kelle and pllim October 9, 2024 20:19
@jeffjennings
Copy link
Contributor Author

jeffjennings commented Oct 9, 2024

I need a review to merge. Please feel free to merge and delete branch once reviewed, thanks! [Never mind, auto-merge is enabled once checks pass.]

Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary though? Are you using setuptools_scm ?

Copy link
Member

Choose a reason for hiding this comment

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

from .version import version as __version__ should have been enough. You can throw in setting __version__ to empty string on ImportError if you want. But should not need special handling like this anymore.

Copy link
Contributor Author

@jeffjennings jeffjennings Oct 10, 2024

Choose a reason for hiding this comment

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

I see. There is no version.py file, so from .version import version as __version__ would require creating one. But for now I'm not looking to rewrite the project setup files, just trying to make minor changes such that learn-astropy workflows that depend on this package can succeed. Within that scope, I think the solution I have is ok for the moment, as it's slightly altering the previous version of the code (even if that wasn't done in the optimal way), and using a core python package to do so. An issue in this repo for updating the setup files would be useful, but right now I'd like to focus on getting the learn pipeline to work again.

Copy link
Member

Choose a reason for hiding this comment

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

version.py should be generated by setuptools_scm during the build process, so that file should be there by the time an import happens. But I think it's not configured correctly -- try adding:

[project]
dynamic = ["version"]

[tool.setuptools_scm]
version_file = "astropylibrarian/_version.py"

to the pyproject.toml file. Then, the code in __init__.py can be what @pllim suggested:

try:
    from ._version import version as __version__
except ImportError:
    __version__ = ""

Copy link
Member

Choose a reason for hiding this comment

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

Oh but doing this means we'll also need to merge the setup.cfg data into the pyproject.toml file, or at least the relevant config items under [project].

Copy link
Member

Choose a reason for hiding this comment

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

If you don't want pyproject.toml , you can achieve the same effect with just setup.cfg (deprecated but should still work)

Copy link
Member

Choose a reason for hiding this comment

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

I guess I'll leave it up to you two if this PR is okay as is or more changes needed. Thanks!

Copy link
Contributor Author

@jeffjennings jeffjennings Oct 10, 2024

Choose a reason for hiding this comment

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

As I said, my focus here is not to rewrite the setup files, it is an atomic change to fix downstream issues in getting the learn website to build. I have opened #37 to update the setup files. Please approve this so I can focus on the higher priority of the learn website.

@pllim pllim disabled auto-merge October 9, 2024 21:10
@pllim
Copy link
Member

pllim commented Oct 9, 2024

I disabled auto merge because I have questions. Thanks!

@adrn adrn merged commit bf4ac2e into main Oct 10, 2024
6 checks passed
@adrn adrn deleted the pkg_resources branch October 10, 2024 14:20
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.

Complete transition of setup and versioning from setup.py to pyproject.toml
3 participants