search

PyO3 / pyo3

Rust bindings for the Python interpreter
https://pyo3.rs
Apache License 2.0
12.06k stars 744 forks source link

add pyclass `eq` option #4210

Closed Icxolu closed 4 months ago

Icxolu commented 4 months ago

In https://github.com/PyO3/pyo3/pull/4206#issuecomment-2132214807 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 eq option, 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)

Icxolu commented 4 months ago

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 int_enum, since that kind of implies interop/subclassing from IntEnum which I believe this does not. Maybe eq_int could be used. An idea could also be to require PartialEq<int_repr> and give a way to autogenerate these.

Icxolu commented 4 months ago

I've added the eq_int option for simple enums. This option is currently treated as always on, but emits a deprecation warning if not user specified. It generates a __richcmp__ implementation that is compatible with the current behavior. If eq is also specified, the comparison using PartialEq is tried first and if the extraction fails, it falls back to integer comparison of the enum discriminants.

davidhewitt commented 4 months ago

Overall this is looking good to me and I agree with the choice of eq_int. Needs a changelog entry, and I'll aim to give a final read through tonight; apart from that, I'm excited to see this merge! Thanks 🙌

Icxolu commented 4 months ago

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 20240514.3 -> 20240526.2. All the actions seem to be exactly the same...

davidhewitt commented 4 months ago

Whatever the cause, it seems to have cleared again now without effort on our part 🚀