Conversation
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| // TODO: `ord` can be integrated here (#4202) |
There was a problem hiding this comment.
I think it should be straightforward to implement the options into this logic. However, I do have a general design question. In my PR I had allowed the old eq and ne logic to be used in conjunction with the newer comparison arms for all other comparisons for simple enums, since equality is required for ordering. With this eq implementation now available, I'm in favor of not allowing the mixing and requiring the eq be implemented if ord is passed (which aligns with ParialOrd requiring ParialEq). It simplifies the logic too. Does that seem reasonable?
There was a problem hiding this comment.
I think it should be straightforward to implement the options into this logic.
Yes, there should be no problem with integrating the ordering here, I just left it open to either fill it out if #4202 lands first, or for refactoring of it, if we land this one first 👍
I'm in favor of not allowing the mixing and requiring the eq be implemented if ord is passed (which aligns with ParialOrd requiring ParialEq). It simplifies the logic too. Does that seem reasonable?
I agree, it just makes sense to require that (and planned to add a check if #4202 lands first 😇)
There was a problem hiding this comment.
Sounds good! Hopefully we can land this PR first. I feel like we should have completed eq first before jumping to ord, but hindsight always gives this clarity. 😅
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks, this implementation looks nice!
Regarding dropping the integer comparison for simple enums, I'm torn. I agree it doesn't really make sense in the context of a PartialEq implementation, but at the same time it does match Python behaviour for simple enums:
>>> from enum import IntEnum
>>> class Color(IntEnum):
... Red = 0
... Blue = 1
... Green = 2
...
>>> Color.Red == 0
True
>>> Color.Red == 1
FalseI wonder, are there design levers we can explore here? For example, whether an enum should have integer comparison might be affected by whether it actually names the discriminants:
#[pyclass]
enum Foo {
A = 1,
B = 2,
}
#[pyclass]
enum Bar {
A,
B,
}One could argue it feels natural for Foo to support integer comparison, but not Bar.
Perhaps rather than doing anything too magical, we should just add the integer comparisons back with another flag? e.g. #[pyclass(eq, int_enum)]
🤔
|
Yeah, I thought so. Dropping the integer comparison without any alternative will be a usability regression. I like the idea of using another option to archive that, but I would not call it |
|
I've added the |
|
Overall this is looking good to me and I agree with the choice of |
|
CI failure seems unrelated, not sure whats causing it... Edit: The only difference I can see is the macos-14-arm64 runner image changed from |
|
Whatever the cause, it seems to have cleared again now without effort on our part 🚀 |
davidhewitt
left a comment
There was a problem hiding this comment.
Looks great to me, thanks! Just a few docs suggestions.
In #4206 (comment) we decided that
#[pyclass(hash)]should also require#[pyclass(eq)], so this implements that option.Special care needs to be taken of simple enums, since they currently always implement equality using their numeric discriminant. This PR keeps that behavior in the absense of the
eqoption, but emits a deprecation warning in that case. The new behavior does not allow direct comparisons between an enum variant and an integer anymore, are we fine with that? (The enum tests still need adapting)