Skip to content
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: __index__ on Enum should always be present. #3700

Merged
merged 3 commits into from
Feb 8, 2022

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Feb 8, 2022

Description

Tiny bit of minor cleanup.

Suggested changelog entry:

* Enum now has an ``__index__`` method on Python <3.8 too

include/pybind11/pybind11.h Outdated Show resolved Hide resolved
@henryiii henryiii changed the title chore: minor odd py version cleanup fix: __index__ on Enum should always be present. Feb 8, 2022
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same question last night (but no chance to ask here). I agree with the conclusion!

If you think it's useful to mention in the PR description or changelog: #2801 also "backported" 3.8 behavior. The PR description there mentions another situation where that was the case.

@henryiii
Copy link
Collaborator Author

henryiii commented Feb 8, 2022

This shouldn't change behavior, since __int__ and __index__ have exactly the same implementation, so the pre/post behavior of #2801 should still be the same here. The only change is enums now always have an __index__, while before they only had one in Python 3.8+; before 3.8 you couldn't use h[enum], but after 3.8 you could.

@henryiii henryiii merged commit af056b6 into pybind:master Feb 8, 2022
@henryiii henryiii deleted the henryiii/chore/hexclean branch February 8, 2022 16:47
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 8, 2022
@rwgk
Copy link
Collaborator

rwgk commented Feb 15, 2022

I believe (!) this PR broke a couple Google-internal tests. I don't have time to fully dig into the details, but what I see is that generated .pyi files now have extra def __index__(self) -> int: ... lines.

@henryiii it might be good to add a warning to the upgrade guide.

@henryiii
Copy link
Collaborator Author

I'd first see if it's just that an extra line is being added to a .pyi file - that's not a breaking change unless you are testing the exact methods and making sure a new dunder method is not added (like by comparing string output in such a file); if you simply upgrade Google from Python 3.7 to Python 3.8, these extra lines would have started showing up as well. Now it's just consistent across Python versions.

@henryiii
Copy link
Collaborator Author

Let me know if Python behavior actually changed, I still think that's really unlikely.

@rwgk
Copy link
Collaborator

rwgk commented Feb 15, 2022

Let me know if Python behavior actually changed, I still think that's really unlikely.

In the meantime the affected team accepted the pybind11 update (and the side-effect changes in their .pyi files) without hesitation. I'll move on with this belief for now: they are harvesting pybind11 docstrings to build their .pyi files. It's actually the same set of .pyi files that was affected when I tested PR #2621 a couple weeks ago, which adds to my belief.

Yes, I agree there will likely be issues with the .pyi files again when we upgrade our Python version.

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants