-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Fix compatibility with Python 3.10, refs #358 #359
Conversation
@@ -43,7 +43,7 @@ jobs: | |||
needs: lint | |||
strategy: | |||
matrix: | |||
python-version: [3.6, 3.7, 3.8, 3.9] | |||
python-version: ["3.6", "3.7", "3.8", "3.9", "3.10"] |
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.
Necessary to use strings rather than floating point numbers here, because otherwise 3.10 is interpreted as 3.1
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 only wrap "3.10" and keep others as is.
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 think it's better to wrap them all for consistency, but have no strong opinion on this as long as it would not make future confusion.
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.
Both variants work technically.
I prefer the current version; if we process 3.10
as a string -- why others should be numbers? Again, technically the version is not a number but a string.
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 but I'll leave this to the maintainers to merge.
Codecov Report
@@ Coverage Diff @@
## master #359 +/- ##
==========================================
- Coverage 95.52% 95.45% -0.07%
==========================================
Files 4 4
Lines 1274 1277 +3
Branches 84 86 +2
==========================================
+ Hits 1217 1219 +2
Misses 50 50
- Partials 7 8 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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've confirmed the symptom reported in #358 and that this PR well handles it.
@@ -43,7 +43,7 @@ jobs: | |||
needs: lint | |||
strategy: | |||
matrix: | |||
python-version: [3.6, 3.7, 3.8, 3.9] | |||
python-version: ["3.6", "3.7", "3.8", "3.9", "3.10"] |
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 think it's better to wrap them all for consistency, but have no strong opinion on this as long as it would not make future confusion.
@asvetlov Could you review & merge this PR to release a new version? |
Thanks! |
What do these changes do?
Tests now also run against Python 3.10, and code incorporates a workaround for a bug in Python 3.10 that affects Janus.
Related issue number
Checklist
CHANGES
folder (skipped, not sure how to do this)