Skip to content

enum: Add iteration and simple type-based check, towards PEP 435 #2744

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

EricCousineau-TRI
Copy link
Collaborator

Description

(Currently draft)
Resolves #2332

Duck-type, but does not support isinstance(pybind11_enum_value, enum.Enum).

Suggested changelog entry:

enum: Add iteration and simple type-based check, towards PEP 435

if enum:
assert not isinstance(value, enum.Enum)
assert value.name == name
assert value.value == raw_value
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Self-note: Should try to implement Flag based interface. (i.e. closure under arithmetic operations)

return iter(cls.__members__.values())

def __len__(cls):
return len(cls.__members__)

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, didn't realize this should be done in enum.EnumMeta, not here. 🤦

Is there a way to do it without altering builtin classes?

Copy link
Collaborator Author

@EricCousineau-TRI EricCousineau-TRI Dec 31, 2020

Choose a reason for hiding this comment

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

Not that I'm aware of :( I had gone the route of modifying builtin classes, but yuck:
master...a41944c
(I didn't get it fully working, hit some weird recursion thing, and gave up 'cause ewww)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it does look too hacky and doesn't sound like right thing to do.

Actually pybind-enum is NOT enum.Enum, so maybe we shouldn't try to make this check return true? I think it would be more appropriate to introduce collections.abc.Enum (e.g. in python 3.10) and make isinstance(pybind_enum_value, colletions.abc.Enum) recognize all PEP-complaining enum implementations? In meantime one probably have no choice other than stick to self-written check method (like one in tests in this PR).

I think such doubtful hacks would greatly reduce the PR merge chances, so I would leave attempts to introduce them, at least for now.

@YannickJadoul
Copy link
Collaborator

Urgh, I didn't realize this would be so much harder than it seems, when reading the original comments in #2332 :-|

@EricCousineau-TRI
Copy link
Collaborator Author

Urgh, I didn't realize this would be so much harder than it seems [...]

Yeah lol, gotta get them meta classes in :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants