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

enum: Add Enum.value property #2739

Merged
merged 3 commits into from
Dec 31, 2020

Conversation

EricCousineau-TRI
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI commented Dec 23, 2020

Description

Towards #2332 - adds a .value attribute

Suggested changelog entry:

enum: add missing Enum.value property

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

This looks fine in general. However, shouldn't we be looking at python3.4 enums, as in #2704 ? Yes, "what about python2" is a good argument, but even pip is dropping support for python2 in January.

include/pybind11/pybind11.h Outdated Show resolved Hide resolved
@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Dec 23, 2020

EDIT: Moved here: #2332 (comment)

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Dec 23, 2020

Er, also - I shoulda put that design comment in the issue.
Moving it there, so further discussion doesn't block this relatively simple PR.

EDIT: Done.

@EricCousineau-TRI
Copy link
Collaborator Author

@bstaletic Any chance you got a brief few min to approve the latest changes here?

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. This looks perfectly fine.

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks, @EricCousineau-TRI!

Are you also planning to tackle the __iter__ part of #2332?

@YannickJadoul YannickJadoul added this to the v2.6.2 milestone Dec 31, 2020
@YannickJadoul
Copy link
Collaborator

Let's see if this still makes it into 2.6.2? I guess it could conflict with users' code when they already defined name(), though, but ... shouldn't break stuff?

@YannickJadoul
Copy link
Collaborator

Are you also planning to tackle the __iter__ part of #2332?

I should read my emails better, and notice #2744...

@EricCousineau-TRI
Copy link
Collaborator Author

Let's see if this still makes it into 2.6.2? I guess it could conflict with users' code when they already defined name(), though, but ... shouldn't break stuff?

Hopefully it's sufficiently benign! It does modify the class defined by py::enum_<CppEnum>, so it could interrupt downstream code. But hopefully,that's not too much of an issue (given that Enum.name already existed).

My vote is, onwards! It cool if I merge?! 😬

@YannickJadoul
Copy link
Collaborator

As far as I'm concerned, yes :-)

Should we quickly check with @henryiii on the 2.6.2 thing; he's been unofficially managing the last few releases?

@EricCousineau-TRI
Copy link
Collaborator Author

Sounds good! Henry, you cool with pushing the merge button on this one?

@henryiii
Copy link
Collaborator

I've edited the changelog a bit to make it sound a bit more like a patch-worthy addition. ;)

@henryiii henryiii merged commit 2110d2d into pybind:master Dec 31, 2020
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 31, 2020
@YannickJadoul
Copy link
Collaborator

Thanks for picking this up, @EricCousineau-TRI!

@EricCousineau-TRI
Copy link
Collaborator Author

Sweet, thank y'all for reviewing 'n merging!

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.

4 participants