-
Notifications
You must be signed in to change notification settings - Fork 78
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
Adds a quick CI job to verify python minimum version #5272
Adds a quick CI job to verify python minimum version #5272
Conversation
We've had a few recent issue/PRs dealing with python minimum version support: deephaven#5227, deephaven#5235, and deephaven#5271 Ultimately, we'd like a full matrix of testing support: deephaven#3724, deephaven#3725 In the more immediate term, there is a tool that I've verified is capable of catching these sorts of issues: https://github.com/netromdk/vermin. I've verified that it does catch deephaven#5227 and deephaven#5271, but does not catch deephaven#5235 (which seems like it is more of a runtime error). This PR adds a quick CI job to verify a python minimum version of 3.8. The `# novermin` comment is necessary in a few locations where we've explicitly / manually worked around python minimum version support.
I've explicitly run this against main expecting it to fail so we can see what it looks like. Once #5271 merges, I'll merge to get up-to-date with main and re-run. |
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 like this a lot. Leaving to a Python-focused reviewer to approve.
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.
LGTM
with: | ||
python-version: '3.12' | ||
cache: 'pip' |
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 wonder if we use 3.8 here, would it catch more 3.8 related compatibility issue?
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 project has this recommendation:
It is recommended to use the most recent Python version to run Vermin on projects since Python's own language parser is used to detect language features, like f-strings since Python 3.6 etc.
Unless we discover otherwise, I'd like to follow this recommendation.
We've had a few recent issue/PRs dealing with python minimum version support: #5227, #5235, and #5271
Ultimately, we'd like a full matrix of testing support: #3724, #3725
In the more immediate term, there is a tool that I've verified is capable of catching these sorts of issues: https://github.com/netromdk/vermin. I've verified that it does catch #5227 and #5271, but does not catch #5235 (which seems like it is more of a runtime error).
This PR adds a quick CI job to verify a python minimum version of 3.8. The
# novermin
comment is necessary in a few locations where we've explicitly / manually worked around python minimum version support.