Skip to content

Handle NotImplemented correctly in ErrorDetail.__ne__#8538

Merged
lovelydinosaur merged 1 commit intoencode:masterfrom
allanlewis:7433-handle-not-implemented-for-ErrorDetail-__ne__
Aug 1, 2022
Merged

Handle NotImplemented correctly in ErrorDetail.__ne__#8538
lovelydinosaur merged 1 commit intoencode:masterfrom
allanlewis:7433-handle-not-implemented-for-ErrorDetail-__ne__

Conversation

@allanlewis
Copy link
Contributor

@allanlewis allanlewis commented Jun 29, 2022

As per the sole commit:

PR #7531 resolved issue #7433 by updating ErrorDetails.__eq__ to correctly handle the NotImplemented case. However, Python 3.9 continues to issue the following warning:

DeprecationWarning: NotImplemented should not be used in a boolean context

This is because __ne__ still doesn't handle the NotImplemented case correctly. In order to avoid this warning, this commit makes the same change for __ne__ as previously made for __eq__.

refs #7433 #7531

@allanlewis
exceptions.ErrorDetail: Handle NotImplemented correctly in __ne__
c59e8cc 
PR #7531 resolved issue #7433 by updating `ErrorDetails.__eq__` to correctly
handle the `NotImplemented` case. However, Python 3.9 continues to issue the
following warning:

    DeprecationWarning: NotImplemented should not be used in a boolean context

This is because `__ne__` still doesn't handle the `NotImplemented` case
correctly. In order to avoid this warning, this commit makes the same change
for `__ne__` as previously made for `__eq__`.
@allanlewis allanlewis mentioned this pull request Jun 29, 2022
Closed
6 tasks
@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Jun 29, 2022

Wait. Haven't we got both of these wrong? In what context is NotImplemented returned?
Surely it should be raised as an exception.

What code is returning it?

lovelydinosaur
lovelydinosaur reviewed Jun 29, 2022
View reviewed changes
Copy link
Contributor

@lovelydinosaur lovelydinosaur left a comment

Choose a reason for hiding this comment

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

Test - does this unapprove?

@allanlewis
Copy link
Contributor Author

allanlewis commented Jun 29, 2022

Test - does this unapprove?

You need to "dismiss review" from the panel at the bottom.

Wait. Haven't we got both of these wrong? In what context is NotImplemented returned?
Surely it should be raised as an exception.

What code is returning it?

No, we've got this right, see the docs:

NotImplemented
A special value which should be returned by the binary special methods (e.g. __eq__(), __lt__(), __add__(), __rsub__(), etc.) to indicate that the operation is not implemented with respect to the other type; may be returned by the in-place binary special methods (e.g. __imul__(), __iand__(), etc.) for the same purpose. It should not be evaluated in a boolean context. NotImplemented is the sole instance of the types.NotImplementedType type.
...
Changed in version 3.9: Evaluating NotImplemented in a boolean context is deprecated. While it currently evaluates as true, it will emit a DeprecationWarning. It will raise a TypeError in a future version of Python.

@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Jun 29, 2022

Gotcha, thanks.

@allanlewis
Copy link
Contributor Author

allanlewis commented Jun 29, 2022

As I said in #7433 (comment) I'm happy to add a test for this case, given some guidance.

@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Jun 29, 2022

Perhaps we're okay with it not having one.
PR #7531 didn't.
Consistency is a virtue, no?

@wieczorek1990
Copy link
Contributor

wieczorek1990 commented Jul 30, 2022

Please rename „r” to „result” or something else.

@lovelydinosaur lovelydinosaur merged commit 224168a into encode:master Aug 1, 2022
@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Aug 1, 2022

@wieczorek1990 The change here is consistent with the existing code above.

@allanlewis
Copy link
Contributor Author

allanlewis commented Aug 1, 2022

Thanks, @tomchristie 🙏

@allanlewis allanlewis deleted the 7433-handle-not-implemented-for-ErrorDetail-__ne__ branch August 1, 2022 14:25
@wieczorek1990
Copy link
Contributor

wieczorek1990 commented Aug 1, 2022

I don’t think short names are good code.

@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Aug 1, 2022

Generally agreed.
A pull request changing them for both cases would be accepted.

@wieczorek1990
Copy link
Contributor

wieczorek1990 commented Aug 1, 2022

@tomchristie Here: #8585

sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
@allanlewis @sigvef
exceptions.ErrorDetail: Handle NotImplemented correctly in __ne__ (…
7346684 
…encode#8538)

PR encode#7531 resolved issue encode#7433 by updating `ErrorDetails.__eq__` to correctly
handle the `NotImplemented` case. However, Python 3.9 continues to issue the
following warning:

    DeprecationWarning: NotImplemented should not be used in a boolean context

This is because `__ne__` still doesn't handle the `NotImplemented` case
correctly. In order to avoid this warning, this commit makes the same change
for `__ne__` as previously made for `__eq__`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lovelydinosaur lovelydinosaur lovelydinosaur approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@allanlewis @lovelydinosaur @wieczorek1990