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 pkg_resources with importlib.metadata; bump minor version number #3

Merged
merged 2 commits into from
Mar 30, 2024

Conversation

kpinc
Copy link
Contributor

@kpinc kpinc commented Mar 30, 2024

Hi,

Thanks for this package.

pkg_resources is depreciated. importlib.metadata replaces it.

I ran into PyYAML issue #601: yaml/pyyaml#601
The 5.4 .* releases all have the problem, so I bumped the required PyYAML version. PyYAML claims to support Python >=3.6, so that should not cause problems with your supported Python version. I can't say if this change would cause any other issues.

I can't say I know what I'm doing when it comes to python packaging. I'm also not sure that I properly ran the regression tests. I did run them and they passed, but only on Debian 12 (bookworm) with Python 3.11 (in a virtual environment containing the latest poetry and other tooling). (You see two commits here because I goofed the regression test run, at first.)

It is probably a good idea to bump the pyramid-helloworld required package version from 0.1.2. The current version is 1.0.0, and I sent in a PR that removes pkg_resources and bumps the pyramid-helloworld version to 1.1.0. In any case, leaving the required pyramid-helloworld version untouched in this package avoids some sort of reflexive mutual conflict which might prevent both from running their tests.

Notes on backported standard library modules:

I like to conditionally import backported standard library modules. I like to see the version number in the code so the version number is explicit, and in an obvious place. That way it is clear when it is time to remove the conditional import and just use the standard library.

I find that creating a class to act as the top-level module in the module path, when the backported library is not top-level, helps with mocks when unit testing.

@kpinc
Copy link
Contributor Author

kpinc commented Mar 30, 2024

A related questions are:
Why is only the "egg" scheme supported?
Given that importlib.metadata is not tied to setuptools, perhaps that requirement can be dropped?
If so, would there be any restrictions on the scheme?

description = "A plaster plugin to configure pyramid app with Yaml"
readme = "README.md"
authors = ["Guillaume Gauvrit <guillaume@gauvr.it>"]
include = ["CHANGELOG.md"]
license = "BSD-derived"

[tool.poetry.dependencies]
# See: https://pypi.org/project/backports.entry-points-selectable
importlib_metadata = { version = ">=3.6", python = "<3.10" }
Copy link
Owner

Choose a reason for hiding this comment

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

I was waiting to drop the python 3.7 support to remove pkg_resources usage.

Is it ok for you to drop python 3.7 support ?

importlib.metadata is avaible in python 3.8
so updating the ligne 13 with the code below should be enough.

python = "^3.8"


setattr(importlib, "metadata", importlib_metadata)
else:
import importlib.metadata
Copy link
Owner

Choose a reason for hiding this comment

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

this is why I dislike that. 11 lines of code to retrieve a version number. this is outstanding.

please remove python 3.7 support and everything will be much simpler.

@mardiros
Copy link
Owner

egg

Honnestly, can't tell.

Why is only the "egg" scheme supported?

I've created this module years ago and I can't remember.
I've read and adapt code from plaster or maybe pyramid. By the way, I've always use egg format
for the entrypoint. I even don't know if we can something else relevant here.

Given that importlib.metadata is not tied to setuptools, perhaps that requirement can be dropped?

Yes, completely

If so, would there be any restrictions on the scheme?

No, egg means that the data to read is in the package metadata and it is not related to setuptools
but to python packaging format itself.

@kpinc
Copy link
Contributor Author

kpinc commented Mar 30, 2024 via email

@mardiros mardiros merged commit a0ea344 into mardiros:master Mar 30, 2024
@mardiros
Copy link
Owner

I will fix it it the main branch and release it

@mardiros
Copy link
Owner

I've release a 1.0.0 version here. let me know if something is wrong in a new issue.

Thanks for the contribution.

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.

None yet

2 participants