Fix comparison methods for python3 #1538

nils-van-zuijlen · 2019-02-17T11:27:08Z

Rebased to merge in 0.10, original PR : #1537

See https://portingguide.readthedocs.io/en/latest/comparisons.html#rich-comparisons for an explaination of why it was broken.

peternewman

Some specific comments.

Also generally, I wonder if we're better off implementing cmp and then reusing that elsewhere, so the actual comparison is only detailed once, rather than having the things being compared in lots of places.

Although apparently cmp is deprecated...

tools/rdm/TestState.py

AUTHORS

python/ola/ClientWrapper.py

python/ola/PidStore.py

tools/rdm/TestDefinitions.py

peternewman · 2019-02-17T14:16:22Z

You'll need to cherry-pick this commit to fix some of the Codespell issues:
34c9433

And 458fbb9

And others which are probably in #1532, but likely jumbled with other bits unfortunately.

nils-van-zuijlen · 2019-02-17T15:27:05Z

But cherry picking commits from master will make horrible merge conflicts when you will merge master in 0.10 (or the opposite).

Maybe I should go back to merging it into master, no ?

peternewman · 2019-02-17T19:02:27Z

But cherry picking commits from master will make horrible merge conflicts when you will merge master in 0.10 (or the opposite).

Maybe I should go back to merging it into master, no ?

My git-fu is probably rubbish, but I normally seem to get away with it, I've done it a few times. It just depends where people are working when some of those CI type things pop up where they get fixed first.

I'd like to get the bugfix into 0.10 if possible.

functools.total_ordering is only available from python 3.2 fix flake8 formatting

When you call RegisterUniverse with action set to UNREGISTER, you don't want to be forced to provide a data_callback

nils-van-zuijlen · 2019-02-21T07:59:19Z

Normally I have addressed all the changes you requested. You may merge now that Travis is on a happy mood 😄

peternewman

A few more comments sorry.

python/ola/ClientWrapper.py

python/ola/OlaClient.py

python/ola/PidStore.py

python/ola/ClientWrapper.py

python/ola/PidStore.py

As stated in the doc (https://docs.python.org/2.7/reference/datamodel.html?highlight=cmp#object.__cmp__) __cmp__ is only called if rich comparison is not defined and we define it as it is the only way to support python 3

nils-van-zuijlen · 2019-02-22T11:08:41Z

It works !
Does it do what you wanted ?

peternewman

A few minor comments.

Other than that, could we add some tests for these new comparisons please. You can probably use existing objects used elsewhere for the tests.

python/ola/ClientWrapper.py

python/ola/OlaClient.py

python/ola/PidStore.py

nils-van-zuijlen · 2019-02-24T19:57:59Z

python/ola/MACAddress.py

@@ -51,18 +51,37 @@ def __hash__(self):
  def __repr__(self):
    return self.__str__()

-  def __cmp__(self, other):


I wonder if this class should implement __hash__ because anyone could write code like this which would change the value of the class :

address = MACAddress(bytearray([0x01, 0x23, 0x45, 0x67, 0x89, 0xab])) # creates a MACAddress mac = address.mac_address # gets its representation inside the class print(hash(address)) # print the class' hash value, for example 1043151800894266584 mac[0] = 0xff # changes the MAC address, shouldn't be possible but it is. print(hash(address)) # prints the class' hash value, different from the first one, for example 6608185319733256019

And would get different results at the two print statements, without violating any python (implicit) rules.

Yep, this agrees it's bad:
https://stackoverflow.com/questions/13041352/python-quickly-hash-mutable-object

Going id based would solve our issue wouldn't it? Even if someone fiddles with the value, it's still the same object? Probably not ideal, but at least safer.

We should still try and fix this please. It looks like a Python module just hashes the value:
https://github.com/drkjam/netaddr/blob/rel-0.7.x/netaddr/eui/__init__.py#L550-L552

But I'd agree we should do something or switch to "id".

python/ola/MACAddress.py

peternewman

Just a few small comments please @nils-van-zuijlen . Also could we have some tests of all this shiny new functionality please.

python/ola/OlaClient.py

And add a comment for the future, when 2.7 will not be supported anymore and we will be free to use functools.total_ordering as we want, without hitting recursion limits.

peternewman

Some minor bits, sorry I dropped the ball on this completely @nils-van-zuijlen

Fancy adding some tests too?

peternewman · 2019-08-02T13:38:31Z

python/ola/MACAddress.py

-      return 1
-    return cmp(self.mac_address, other.mac_address)
+  def __eq__(self, other):
+    if other.__class__ is not self.__class__:


Do we need to do:
if other is None or other.__class__ is not self.__class__:

To protect against other being None, or does None still have class?

On the topic of None.__class__, anecdotally it has __class__:

$ python2 Python 2.7.15+ (default, Nov 27 2018, 23:36:35) [GCC 7.3.0] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> None.__class__ <type 'NoneType'>

$ python3 Python 3.6.8 (default, Jan 14 2019, 11:02:34) [GCC 8.0.1 20180414 (experimental) [trunk revision 259383]] on linux Type "help", "copyright", "credits" or "license" for more information. >>> None.__class__ <class 'NoneType'>

isinstance(other, self.__class__) would take care of this for you.

If isinstance isn't desired, it seems like a more bulletproof approach might be to wrap these comparisons in a try except AttributeError block, because it is ambiguous in the documentation of __class__ which instances have __class__. In Python 3 a value not having a __class__ attribute is an even more distant edge case.

Is there some reason this doesn't this return NotImplemented like the other operators?

It doesn't return NotImplemented because if classes don't match, the two objects aren't equals.

Do you want to switch to isinstance @nils-van-zuijlen or is there some reason not to do so?

peternewman · 2019-08-02T13:40:10Z

python/ola/MACAddress.py

    return self.mac_address < other.mac_address

-  def __eq__(self, other):
+  # These 4 can not be replaced with functools.total_ordering because of the


Would this be the same that they could "be replaced with functools.total_ordering when support for 2.7 is dropped because NotImplemented is not supported by it before 3.4"? Or even when we're on Python 3.4 will functools.total_ordering still not be sufficient?

No, we need to implement these functions ourselves because we have a special treatment applied to None that functools.total_ordering doesn't support.

Is this because otherwise sorting None and a MAC you'd get stuck or something? Can we have a bit of a comment explaining why in the code please @nils-van-zuijlen ?

peternewman · 2019-09-29T00:36:12Z

Sorry @nils-van-zuijlen I've been rubbish again. Any chance of some tests? Apart from that it essentially looks good to go thanks!

brucelowekamp · 2019-12-27T19:55:56Z

I have tests for most of these written and should be able to submit a PR in a few days, though I'm not sure if I can add to this or need to open a new one.

@peternewman is there a need to support python older than 2.7? Quite a bit is simpler if we can assume 2.7 and recent 3.

@nils-van-zuijlen 2.7 functools appears to have backports to support everything needed here

A couple more fixes and bugs found with the tests...

peternewman · 2020-04-28T00:46:20Z

@nils-van-zuijlen or @brucelowekamp I think this can be closed now looking at the comparison:
0.10...nils-van-zuijlen:0.10-python3

Would you agree this has all gone in via #1605?

nils-van-zuijlen · 2020-04-28T09:12:19Z

Yes it can be closed, all meaningful commits are in #1605

peternewman · 2020-04-29T15:58:44Z

Perfect and thanks again for your efforts @nils-van-zuijlen !

Fix comparaison methods for python3

be91ba2

See https://portingguide.readthedocs.io/en/latest/comparisons.html#rich-comparisons for an explaination of why it was broken.

nils-van-zuijlen mentioned this pull request Feb 17, 2019

Fix comparison methods for python3 #1537

Closed

peternewman mentioned this pull request Feb 17, 2019

Python 3 Compatibility #1506

Open

peternewman requested changes Feb 17, 2019

View reviewed changes

Fix compat for python 2.6

63821b9

functools.total_ordering is only available from python 3.2 fix flake8 formatting

nils-van-zuijlen force-pushed the 0.10-python3 branch from 79e0b5b to 63821b9 Compare February 19, 2019 17:31

nils-van-zuijlen and others added 4 commits February 19, 2019 18:47

Comparison fixes

3d9d8d8

More codespellignore

ea4fe8c

Update some PID names to fix some spelling mistakes

85d1815

Python OlaClient.RegisterUniverse fix

34aa0b1

When you call RegisterUniverse with action set to UNREGISTER, you don't want to be forced to provide a data_callback

nils-van-zuijlen force-pushed the 0.10-python3 branch from 40816eb to 34aa0b1 Compare February 19, 2019 18:27

peternewman and others added 8 commits February 19, 2019 19:35

Fix a typo

a3a0d2a

Fix a typo

f35e9f2

Fix a typo

99de05c

Fix a typo

a36a9d7

Fix a typo

3bd33aa

Fix a typo

d1f33df

Fix a typo

b3717db

flake8

bef6278

peternewman requested changes Feb 21, 2019

View reviewed changes

python/ola/ClientWrapper.py Show resolved Hide resolved

python/ola/OlaClient.py Show resolved Hide resolved

python/ola/PidStore.py Outdated Show resolved Hide resolved

python/ola/ClientWrapper.py Show resolved Hide resolved

python/ola/PidStore.py Show resolved Hide resolved

nils-van-zuijlen added 3 commits February 21, 2019 20:05

Remove the unneeded __cmp__ methods

1936e68

As stated in the doc (https://docs.python.org/2.7/reference/datamodel.html?highlight=cmp#object.__cmp__) __cmp__ is only called if rich comparison is not defined and we define it as it is the only way to support python 3

Fix MACAddress

22ab3d3

Fix cross class comparison

94ba60a

peternewman requested changes Feb 24, 2019

View reviewed changes

nils-van-zuijlen commented Feb 24, 2019

View reviewed changes

Add hash functions

4facd09

peternewman reviewed Feb 24, 2019

View reviewed changes

python/ola/MACAddress.py Show resolved Hide resolved

nils-van-zuijlen added 2 commits February 24, 2019 21:49

Simplify "or equal" functions

c44bb45

Code style when comparing multiple values

49ebbd7

peternewman added this to the 0.10.8 milestone Mar 7, 2019

peternewman requested changes Mar 14, 2019

View reviewed changes

python/ola/OlaClient.py Show resolved Hide resolved

python/ola/OlaClient.py Show resolved Hide resolved

python/ola/OlaClient.py Show resolved Hide resolved

python/ola/OlaClient.py Show resolved Hide resolved

python/ola/OlaClient.py Show resolved Hide resolved

nils-van-zuijlen and others added 3 commits March 23, 2019 18:13

Makes the order of comparison methods consistent

a9f9dbe

And add a comment for the future, when 2.7 will not be supported anymore and we will be free to use functools.total_ordering as we want, without hitting recursion limits.

Code spelling

c38df41

Merge branch '0.10' into 0.10-python3

cbbe788

peternewman mentioned this pull request Aug 2, 2019

make it run on python3 #1572

Closed

peternewman requested changes Aug 2, 2019

View reviewed changes

Merge branch '0.10' into 0.10-python3

e6e7c5e

peternewman mentioned this pull request Oct 16, 2019

protobuf@3.1: remove Homebrew/homebrew-core#45042

Closed

5 tasks

peternewman mentioned this pull request Nov 25, 2019

ImportError (google.protobuf) with rdm_test_server.py #1599

Closed

brucelowekamp mentioned this pull request Jan 3, 2020

Py3 cmp #1605

Merged

nils-van-zuijlen closed this Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix comparison methods for python3 #1538

Fix comparison methods for python3 #1538

nils-van-zuijlen commented Feb 17, 2019

peternewman left a comment •

edited

Loading

peternewman commented Feb 17, 2019

nils-van-zuijlen commented Feb 17, 2019

peternewman commented Feb 17, 2019 •

edited

Loading

nils-van-zuijlen commented Feb 21, 2019

peternewman left a comment

nils-van-zuijlen commented Feb 22, 2019

peternewman left a comment

nils-van-zuijlen Feb 24, 2019

peternewman Feb 24, 2019

peternewman Aug 2, 2019

peternewman left a comment

peternewman left a comment

peternewman Aug 2, 2019

mvollrath Aug 5, 2019

nils-van-zuijlen Aug 19, 2019

peternewman Sep 29, 2019

peternewman Aug 2, 2019

nils-van-zuijlen Aug 19, 2019

peternewman Sep 29, 2019

peternewman commented Sep 29, 2019

brucelowekamp commented Dec 27, 2019

peternewman commented Apr 28, 2020

nils-van-zuijlen commented Apr 28, 2020

peternewman commented Apr 29, 2020

Fix comparison methods for python3 #1538

Fix comparison methods for python3 #1538

Conversation

nils-van-zuijlen commented Feb 17, 2019

peternewman left a comment • edited Loading

Choose a reason for hiding this comment

peternewman commented Feb 17, 2019

nils-van-zuijlen commented Feb 17, 2019

peternewman commented Feb 17, 2019 • edited Loading

nils-van-zuijlen commented Feb 21, 2019

peternewman left a comment

Choose a reason for hiding this comment

nils-van-zuijlen commented Feb 22, 2019

peternewman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternewman left a comment

Choose a reason for hiding this comment

peternewman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternewman commented Sep 29, 2019

brucelowekamp commented Dec 27, 2019

peternewman commented Apr 28, 2020

nils-van-zuijlen commented Apr 28, 2020

peternewman commented Apr 29, 2020

peternewman left a comment •

edited

Loading

peternewman commented Feb 17, 2019 •

edited

Loading