PyO3 / pyo3

Rust bindings for the Python interpreter
https://pyo3.rs
Other
11.47k stars 694 forks source link

feat: Add 'ord' option for PyClass and corresponding tests #4202

Closed Zyell closed 3 weeks ago

Zyell commented 1 month ago

Updated the macros back-end to include 'ord' as an option for PyClass allowing for Python-style ordering comparison of enum variants. Additionally, test cases to verify the proper functioning of this new feature have been introduced.

Thank you for contributing to PyO3!

By submitting these contributions you agree for them to be dual-licensed under PyO3's MIT OR Apache-2.0 license.

Please consider adding the following to your pull request:

PyO3's CI pipeline will check your pull request, thus make sure you have checked the Contributing.md guidelines. To run most of its tests locally, you can run nox. See nox --list-sessions for a list of supported actions.

Zyell commented 1 month ago

I just pushed the changes I worked on over the weekend (sorry for the delay! It was a holiday weekend here, didn't get around to finalizing things until this morning) to support ordering for all enums and structs. I've updated tests and placed a note in the documentation. Compile time errors are also validated for when ParialOrd is not implemented and the ord argument is passed to the pyclass macro. I have made notes that additional checks for equality will be added when that is done. @Icxolu I will take a closer look at your PR to merge that. Edit: Found pyclass parameters documentation section, but let me know if additional updates should be made elsewhere.

Zyell commented 1 month ago

@Icxolu I am working on integrating ord now that eq has been merged. I was working on the tests and came across this case:

#[pyclass(eq_int)]
#[derive(Debug, Clone)]
pub enum MyEnumOrd {
    Variant,
    OtherVariant,
}

#[test]
fn test_simple_enum_ord_comparable() {
    Python::with_gil(|py| {
        let var1 = Py::new(py, MyEnumOrd::Variant).unwrap();
        let var2 = Py::new(py, MyEnumOrd::Variant).unwrap();
        py_assert!(py, var1 var2, "(var1 == var2) == True");
    })
}

This currently fails, but adding eq causes it to pass. There was a test in the test suite before with a similar test for which this was passing without eq or eq_int. If the goal is to retain the old behavior while issuing the deprecation warning, shouldn't the above test pass? Is this the expected behavior if only eq_int is passed? I ran this test on main to verify it wasn't some regression I introduced. The only difference I can see is in that no match arms are added, just the catchall remains, and the slot is generated differently here.

Icxolu commented 1 month ago

Oops, yeah that's a bug. Thanks for catching! It seem like I forgot the eq_int case in pyclass_richcmp_arms, I'll open a PR to fix 😇