-
Notifications
You must be signed in to change notification settings - Fork 36
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: Add support for Python 3.11 #329
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There are two changes: Changes in the actual code: - _member_names changed from a list to a dict in python/cpython#28907 - we instance-check and remove by list-specific or dict-specific way Change in the tests only: - accessing other enum members via instance attributes is no longer possible - we access them via the class instead - we leave the original test in a try-except block Some of the Python enum changes might get reverted, see python/cpython#93910 But the fix is backwards compatible. Fixes googleapis#326
Feel free to reword that message however you see fit.
A link would be helpful for this. |
@@ -49,7 +49,11 @@ def test_total_ordering_w_other_enum_type(): | |||
|
|||
for item in enums_test.OtherEnum: | |||
assert not to_compare == item | |||
assert to_compare.SOME_VALUE != item | |||
assert type(to_compare).SOME_VALUE != item |
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.
to_compare
is already OneEnum.SOME_VALUE
-- so what is the purpose of the indirect reference through itself? Can this part of the test just be dropped?
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.
Yeah, this entire assertion here seemed redundant to me as well, but I decided to keep testing what was tested.
Note that I am not available to fine-tune this into perfection, that's why I posted the patch in a comment initially as "this makes it pass" instead of opening a PR right away.
Thanks @hroncok! Please could you sign the CLA using the link below? 📝 If you are not currently covered under a CLA, please visit https://cla.developers.google.com/. |
I'm pretty sure I've already once approved this as being part of Red Hat, but now I also signed as an individual. |
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
The github actions checks appear to be stuck with |
LGTM but let's wait for #327 to be merged so that checks are passing. |
I'm going to try adding python |
The github action failed to start due to a syntax error. It should be fixed now |
There are two changes:
Changes in the actual code:
Change in the tests only:
Some of the Python enum changes might get reverted,
see python/cpython#93910
But the fix is backwards compatible.
Fixes #326