fix Python 3 regressions #71
Conversation
| @@ -226,6 +228,9 @@ def to_time(self): | |||
| """ | |||
| return self.to_sec() | |||
|
|
|||
| def __hash__(self): | |||
| return super(Time, self).__hash__() | |||
There was a problem hiding this comment.
Why are these needed? Doesn't the base class provide them>
There was a problem hiding this comment.
No, this is needed.
There was a problem hiding this comment.
Sorry, clearly the base class does provide them, because we're calling super!
What I meant to say, is why do we need to overload them? This works just fine for me in python 2.7 and 3.5
class Base(object):
def __hash__(self):
return 4
class Derived(Base): pass
class DerivedWithOverride(Base):
def __hash__(self):
return super(DerivedWithOverride, self).__hash__()
assert hash(Base()) == hash(DerivedWithOverride()) == hash(Derived()) == 4So what exactly is adding these lines trying to fix?
There was a problem hiding this comment.
I didn't work for me in the case of the Duration and Time classes. You can try to remove them from this patch and it should fail with:
TypeError: 'NoneType' object is not callable
There was a problem hiding this comment.
Can you give me the full traceback?
There was a problem hiding this comment.
Traceback (most recent call last):
File "/home/dthomas/ros/kinetic/github/ros/genpy/test/test_genpy_rostime.py", line 180, in test_Time
self.test_TVal(TVal=Time, test_neg=False)
File "/home/dthomas/ros/kinetic/github/ros/genpy/test/test_genpy_rostime.py", line 61, in test_TVal
self.assertEquals(v.__hash__(), TVal(0, 0).__hash__())
TypeError: 'NoneType' object is not callable
There was a problem hiding this comment.
Ok, I see it now. Something weird seems to be happening, so in the absence of knowing the reason, reintroducing these lines makes sense!
There was a problem hiding this comment.
Thanks for drawing my attention to this, and sorry for the bad patch before. In case you're interested, the minimal case is http://stackoverflow.com/q/40186509/102441
| @@ -180,7 +180,9 @@ def __ne__(self, other): | |||
| def __cmp__(self, other): | |||
There was a problem hiding this comment.
Arguably the real issue here is implementing __cmp__ in python 3 at all. If cmp doesn't exist, then __cmp__ will never be called. So I don't understand what this patch is fixing.
There was a problem hiding this comment.
Ah, I see what the issue is - the implementation of __lt__ delegates to __cmp__
Regressions from #63.
@eric-wieser FYI