Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix Python 3 regressions #71

Merged
merged 1 commit into from
Oct 21, 2016
Merged

fix Python 3 regressions #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/genpy/rostime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,9 @@ def __ne__(self, other):
def __cmp__(self, other):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see what the issue is - the implementation of __lt__ delegates to __cmp__

if not isinstance(other, TVal):
raise TypeError("Cannot compare to non-TVal")
return cmp(self.to_nsec(), other.to_nsec())
a = self.to_nsec()
b = other.to_nsec()
return (a > b) - (a < b)

def __eq__(self, other):
if not isinstance(other, TVal):
Expand Down Expand Up @@ -226,6 +228,9 @@ def to_time(self):
"""
return self.to_sec()

def __hash__(self):
return super(Time, self).__hash__()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these needed? Doesn't the base class provide them>

Copy link
Member Author

@dirk-thomas dirk-thomas Oct 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

No, this is needed.

Copy link
Contributor

@eric-wieser eric-wieser Oct 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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()) == 4

So what exactly is adding these lines trying to fix?

Copy link
Member Author

@dirk-thomas dirk-thomas Oct 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give me the full traceback?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see it now. Something weird seems to be happening, so in the absence of knowing the reason, reintroducing these lines makes sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

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


def __repr__(self):
return "genpy.Time[%d]"%self.to_nsec()

Expand Down Expand Up @@ -261,7 +266,9 @@ def __cmp__(self, other):
"""
if not isinstance(other, Time):
raise TypeError("cannot compare to non-Time")
return cmp(self.to_nsec(), other.to_nsec())
a = self.to_nsec()
b = other.to_nsec()
return (a > b) - (a < b)

def __eq__(self, other):
"""
Expand Down Expand Up @@ -303,6 +310,9 @@ def __setstate__(self, state):
"""
self.secs, self.nsecs = state

def __hash__(self):
return super(Duration, self).__hash__()

def __repr__(self):
return "genpy.Duration[%d]"%self.to_nsec()

Expand Down Expand Up @@ -423,7 +433,9 @@ def __divmod__(self, val):
def __cmp__(self, other):
if not isinstance(other, Duration):
raise TypeError("Cannot compare to non-Duration")
return cmp(self.to_nsec(), other.to_nsec())
a = self.to_nsec()
b = other.to_nsec()
return (a > b) - (a < b)

def __eq__(self, other):
if not isinstance(other, Duration):
Expand Down
14 changes: 7 additions & 7 deletions test/test_genpy_rostime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,19 +419,19 @@ def test_Duration(self):
self.assertEquals(Duration(4), Duration(8) / 2.)
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter('always')
self.assertEquals(Duration(4), Duration(8) // 2)
self.assertEquals(Duration(4), Duration(8) // 2.)
self.assertEquals(Duration(4), Duration(9) // 2)
self.assertEquals(Duration(4), Duration(9) // 2.)
self.assertEqual(Duration(4), Duration(8) // 2)
self.assertEqual(Duration(4), Duration(8) // 2.)
self.assertEqual(Duration(4), Duration(9) // 2)
self.assertEqual(Duration(4), Duration(9) // 2.)
self.assertEqual(len(w), 0)
self.assertEquals(Duration(4, 2), Duration(8, 4) / 2)
v = Duration(4, 2) - (Duration(8, 4) / 2.)
self.assert_(abs(v.to_nsec()) < 100)

with warnings.catch_warnings(record=True) as w:
warnings.simplefilter('always')
self.assertEquals(Duration(4, 0), Duration(8, 4) // 2)
self.assertEquals(Duration(4, 0), Duration(9, 5) // 2)
self.assertEqual(Duration(4, 0), Duration(8, 4) // 2)
self.assertEqual(Duration(4, 0), Duration(9, 5) // 2)
v = Duration(4, 2) - (Duration(9, 5) // 2.)
self.assert_(abs(v.to_nsec()) < 100)
self.assertTrue(abs(v.to_nsec()) < 100)
self.assertEqual(len(w), 0)