-
Notifications
You must be signed in to change notification settings - Fork 55
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
[MAINT] Fixing Unit Tests for Python 3.11 and 3.12 support #800
Conversation
@gtdang @ntolley @jasmainak @dylansdaniels
The code is looking to raise a
It seems that the regex rules changed in python 3.11+. It is enforced that global flags like
Similar to the fist point, The code is looking to raise an
In this case, pythom 3.11+ changed the message of the Looking forwards for your comments. |
I'm not a regular expression expert but I trust your judgement. Can you run the CIs so I can see that they get fixed? |
Do you mean the CI in the master branch? or in this PR? |
e9156df
to
d900d61
Compare
d900d61
to
1e5efee
Compare
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.
Have one minor suggestion but otherwise approve.
@@ -13,7 +13,7 @@ | |||
strategy: | |||
matrix: | |||
os: [ubuntu-latest,macos-latest] | |||
python-version: [3.8, 3.9, '3.10'] | |||
python-version: [3.8, 3.9, '3.10', 3.11, 3.12] |
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.
python-version: [3.8, 3.9, '3.10', 3.11, 3.12] | |
python-version: [3.8, 3.11, 3.12] |
Really great work getting this working @kmilo9999!
As per our discussion, I think these will be the versions we stick with for now. Definitely will be keeping in mind the idea of having a separate branch for testing the full matrix in the future
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.
Ah nice catch. I was discussing with a colleague that one possibility is to have a separate full-matrix workflow that tests the main branch as a cron job. You can set up a workflow to run on a monthly or 2-week basis or something.
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.
Should I remove 3.9
and 3.10
from the windows workflow too?
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
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.
Done
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.
Looks great!
@@ -13,7 +13,7 @@ | |||
strategy: | |||
matrix: | |||
os: [ubuntu-latest,macos-latest] | |||
python-version: [3.8, 3.9, '3.10', 3.11, 3.12] | |||
python-version: [3.8, 3.11, 3.12] |
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.
could you document the reason for this change on github so future developers know why it was done?
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.
Should we document it at the top of this PR?
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.
doesn't matter as long as it's somewhere on github! If a decision was made during a private meeting, it's generally good practice to document the reasoning on github
@ntolley |
All set! Thanks @kmilo9999 we're all breathing much easier with the return of the green checkmarks 😄 |
✅ 🥳 ✅ 🥳 ✅ |
For now we decided to test the 2 latest versions and oldest supported version of python on mac, Linux, and windows for all PRs. A full-matrix test of all supported python versions performed at a longer frequency is still being considered.
This PR is aimed to fix incompatibilities found in the tests against python 3.11 and 3.12