Fix bug in default implementation of __ne__
#3506
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidhewitt
commented
Oct 11, 2023
Comment on lines
+56
to
57
| @pytest.mark.parametrize("ty", (EqDefaultNe, PyEqDefaultNe), ids=("rust", "python")) | ||
| def test_eq_default_ne(ty: Type[Union[EqDefaultNe, PyEqDefaultNe]]): |
Member
Author
There was a problem hiding this comment.
This was my original bug in the test suite.
Comment on lines
+152
to
+153
| PyO3 supports the usual magic comparison methods available in Python such as `__eq__`, `__lt__` | ||
| and so on. It is also possible to support all six operations at once with `__richcmp__`. |
Member
Author
There was a problem hiding this comment.
protocols.md already covers __eq__ etc so I think it was just this section of the guide which I found needed updating.
Comment on lines
806
to
818
| unsafe fn __ne__( | ||
| self, | ||
| _py: Python<'_>, | ||
| _slf: *mut ffi::PyObject, | ||
| _other: *mut ffi::PyObject, | ||
| py: Python<'_>, | ||
| slf: *mut ffi::PyObject, | ||
| other: *mut ffi::PyObject, | ||
| ) -> PyResult<*mut ffi::PyObject> { | ||
| Ok(ffi::_Py_NewRef(ffi::Py_NotImplemented())) | ||
| // By default `__ne__` will try `__eq__` and invert the result | ||
| let slf: &PyAny = py.from_borrowed_ptr(slf); | ||
| let other: &PyAny = py.from_borrowed_ptr(other); | ||
| if !slf.eq(other)? { | ||
| Ok(ffi::Py_NewRef(ffi::Py_True())) | ||
| } else { | ||
| Ok(ffi::Py_NewRef(ffi::Py_False())) | ||
| } | ||
| } | ||
| } |
Member
Author
There was a problem hiding this comment.
... and this is the bugfix. :)
Member
Author
There was a problem hiding this comment.
(Adjusted to use a simpler implementation in a force-push)
Merged
Member
Author
|
Skip changelog as |
davidhewitt
force-pushed
the
default-ne
branch
from
9a7420e to
76f6c45
Compare
davidhewitt
force-pushed
the
default-ne
branch
from
76f6c45 to
e1d4173
Compare
mejrs
approved these changes
Oct 11, 2023
Member
mejrs
left a comment
mejrs
left a comment
There was a problem hiding this comment.
I just woke up but this seems good to me :)
Member
Author
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #3417
While updating the documentation for
__eq__I fortunately stumbled across an error in the testing for__eq__and__ne__which caused us to miss the fact that__ne__was not defaulting to__eq__the same way it does for Python classes.If anyone is available to give this a quick review (hopefully it's not a difficult review) then I'd be very grateful, as I think this blocks the release.