classification
Title: Fix comparison of plistlib.Data
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: python-dev, ronaldoussoren, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-04-07 20:06 by serhiy.storchaka, last changed 2016-05-01 10:46 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
plistlib_data_eq.patch serhiy.storchaka, 2016-04-07 20:06 review
plistlib_data_eq_2.patch serhiy.storchaka, 2016-04-08 11:44 review
Messages (7)
msg263001 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-07 20:06
Proposed patch fixes several bugs in plistlib.Data.__eq__().

* isinstance(other, str) was used instead of isinstance(other, bytes). Data always wraps bytes and should be comparable with bytes. str was correct type in Python 2.

* id(self) == id(other) is always false, because if other is self, the first condition (isinstance(other, self.__class__)) should be true. NotImplemented should be returned as fallback. This allows comparing with Data subclasses and correct work of __ne__().

* The __eq__() method should be used instead of the equality operator. This is needed for correct work in case if value is bytes subclass with overloaded __eq__().
msg263018 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2016-04-08 10:56
I don't understand the explicit use of __eq__ in the bytes case. Why is that needed?
msg263020 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-08 11:24
>>> import plistlib
>>> class MyData(bytes):
...     def __eq__(self, other):
...         if isinstance(other, plistlib.Data):
...             return super().__eq__(other.value)
...         return False
... 
>>> plistlib.Data(b'x') == MyData(b'x')
True

If use the equality operator the result is False.

I don't know if this is good example. In any case this is corner case and we can manage with "==".
msg264567 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-30 17:21
Ronald, my second patch uses "==" instead of __eq__. Is it good to you?
msg264589 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2016-05-01 08:51
Serhiy,

I slightly prefer the second patch, but either one would be fine.  

The reason I asked about explicitly calling __eq__ is that this is an uncommon pattern (at least for me). 

Ronald
msg264592 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-05-01 10:37
New changeset 41afb83cffac by Serhiy Storchaka in branch '3.5':
Issue #26711: Fixed the comparison of plistlib.Data with other types.
https://hg.python.org/cpython/rev/41afb83cffac

New changeset dbdd5bc4df99 by Serhiy Storchaka in branch 'default':
Issue #26711: Fixed the comparison of plistlib.Data with other types.
https://hg.python.org/cpython/rev/dbdd5bc4df99
msg264593 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-01 10:46
For simplicity I left "==" and fixed only obvious bug.
History
Date User Action Args
2016-05-01 10:46:01serhiy.storchakasetstatus: open -> closed
messages: + msg264593

assignee: serhiy.storchaka
resolution: fixed
stage: patch review -> resolved
2016-05-01 10:37:09python-devsetnosy: + python-dev
messages: + msg264592
2016-05-01 08:51:28ronaldoussorensetmessages: + msg264589
2016-04-30 17:21:03serhiy.storchakasetmessages: + msg264567
2016-04-08 11:44:01serhiy.storchakasetfiles: + plistlib_data_eq_2.patch
2016-04-08 11:24:19serhiy.storchakasetmessages: + msg263020
2016-04-08 10:56:56ronaldoussorensetmessages: + msg263018
2016-04-07 20:06:42serhiy.storchakacreate