Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support Python 3.11, drop older Pythons entirely #59
Support Python 3.11, drop older Pythons entirely #59
Changes from 20 commits
8c70e22
198870f
cc5c60f
8cc3832
73a36f0
208c2fd
0d1d0ca
1827126
fe8014d
0b84709
d5926ba
9aad5c9
bc42a86
97b7e06
e930748
74a386a
69472db
29d806e
b452239
f01db11
4658fe7
450dd3c
25f8de7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This comment was marked as resolved.
Sorry, something went wrong.
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.
Since the base code of this library has no change, I don't wan't to drop those versions.
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.
Is this library used anywhere?
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.
Yes. It's still required for Python 3.7. It can be removed when 3.7 support goes away, which I'd recommend doing when 3.7 is EOL later this year.
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 didn't actually see it used in the code?
Anyway, one suggestion is to use the stdlib version for 3.8+:
'importlib-metadata; python_version < "3.8"',
And then it can be removed when 3.7 is EOL in June: https://devguide.python.org/versions/
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 failed to document why I added it in 1827126 but I did note that it is needed for dev only. I think maybe wrapt needed it?
What's the danger of leaving it now and coming back to removing it? It's a dev dep so it shouldn't affect the public dependencies list.
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.
Ya know, I'll bet I had it in there to consolidate the lines removed below, assuming that
importlib-metadata
was actually used somewhere…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.
No, I want to keep this unchanged.
From my point of view, the reason that it is no longer possible to run unit tests with the recent tools is not a reason to abandon the support of Python 2.7 and the older versions of Python 3. If a user has an old development environment, he will be able to install and use the library.
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.
But surely they could continue using the older releases of this package, if they still rely on python 2.7?
It won't break any user if they are stuck on an older version, because other packages are also not moving and there is no support/security-fixes anyways?
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.
This PR does not change the code base, so the code remains compatible with older versions of Python. I don't want to make a major release for this PR: it's only a build fix.
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.
If you're OK with allowing installation on an untested Python version, I'll revert the versions drop. I myself would not want to receive reports about any future failure on untested and EOL versions but I recognize the risk of breakage is low given that this library doesn't need to change much.
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.
Researching a little deeper, I could retain testing on the older versions by switching the base platform for testing to
all of these per https://github.com/actions/python-versions/blob/main/versions-manifest.json
Looks like I can also enable
3.12.0-alpha - 3.12.0
through the semver hyphen ranges.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.
2.7 is being removed in a couple of weeks actions/runner-images#7401
Instead of
3.12.0-alpha - 3.12.0
, we can use the simpler3.12-dev
. https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#using-the-python-version-inputThere 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 am OK with that.
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.
Do we need to test five versions of wrapt? 1.10 is from 2017, pretty old by now.
If we're adding two, shall we remove two?
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.
Support table:
It looks very safe to drop 1.10 because none of its supported versions would be supported in the next Deprecated release if this PR is merged. I'd call it safe enough to drop 1.11-1.12 and probably even 1.13, tbh.
°: latest in series
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'd say let's keep the wrapt dep and testing at 1.10+ for this PR and if I have time this week (long shot), I can do the work to eliminate 1.10-1.13 to settle on 1.14+.