-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add enum value to enum repr #2126
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
Conversation
f6e83cf
to
b38a0be
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.
Looks good to me! :-)
@@ -1435,29 +1445,22 @@ struct enum_base { | |||
auto static_property = handle((PyObject *) get_internals().static_property_type); | |||
|
|||
m_base.attr("__repr__") = cpp_function( | |||
[](handle arg) -> str { | |||
[](object arg) -> str { |
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.
Why handle to object? You don't need reference counting here, do you?
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.
Argh, good catch. I should've look more closely at the details rather than eyeballing the structural changes and shrugging "OK"...
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.
OK, so I just tried this out, and the reason seems to be because there's no int_
conversion from handle
(you can reinterpret_steal
/reinterpret_borrow
, but those will not do the int(...)
conversion).
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.
See e.g. __getstate__
and __hash__
, also doing that. I don't immediately see a better solution, then? (though if feels there ought to be one, changing the interface of the Python types; but that's a different discussion/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.
Maybe pass the object
by reference, to avoid reference counting?
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.
@bstaletic Hmmm, interesting.
Thinking about it a bit futher: will it, though? I believe (though the cast
, make_caster
, cast_op
, PYBIND11_TYPE_CASTER
, ... code is quite convoluted) that the object
will be created and moved from the caster anyway, if you take it by value. So it shouldn't matter?
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 vote to keep it this way, merge this PR, and create an issue/discussion on why you can't call the int_
conversion on a handle (like you can for str
).
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'm okay with that.
b38a0be
to
77a72c0
Compare
I've taken the liberty to rebase your PR onto the latest |
Successfully rebased, it seems :-) Does @bstaletic still want to say something (as you were involved in the discussion?) Otherwise, this should be ready, @henryiii? |
This changes enum reprs to look like `<Enum.name: value>` similarly to the Python enum module. This keeps the str of enums as `Enum.name`, like the Python enum module.
77a72c0
to
5b8da50
Compare
Another rebase, after #2510. If CI turns green, I'll merge. |
Thanks, @auscompgeek! |
Fixed in pybind#2510 but reintroduced on one line by pybind#2126
This changes enum reprs to look like
<Enum.name: value>
similarly tothe Python enum module.
This keeps the str of enums as
Enum.name
, like the Python enum module.Ref: #2332