add repr for Board and Markers #86

Adimote · 2018-03-06T22:49:26Z

Currently Board and Markers don't show pretty prints if you print(markers). They do have __str__() functions, but print() calls __repr__(), so implement __repr__()

PeterJCLaw

Would be good not to call __str__ directly -- the string formatting will do that for us.

Requesting changes due to test failures.

PeterJCLaw · 2018-03-06T23:55:28Z

robot/markers.py

@@ -168,9 +168,12 @@ def spherical(self) -> SphericalCoord:
        """
        return SphericalCoord(*self._raw_data['spherical'])

+    def __repr__(self):
+        return "<robot.markers.{} >".format(self.__str__())


You shouldn't need to call __str__ explicitly here.

fixed in f6b302c

PeterJCLaw · 2018-03-06T23:57:26Z

robot/board.py

@@ -155,6 +155,9 @@ def close(self):
        """
        self.socket.detach()

+    def __repr__(self):
+        return "<{}>".format(self.__str__())


You shouldn't need to call __str__ explicitly here.

fixed in f6b302c

trickeydan · 2018-03-07T01:07:39Z

robot/markers.py

    def __str__(self):
        bearing = self.spherical.rot_y_degrees
-        return "<Marker {}: {:.0f}° {}, {:.2f}m away>".format(
+        return "Marker {}: {:.0f}° {}, {:.2f}m away".format(
            self.id,
            abs(bearing),
            "right" if bearing > 0 else "left",


Do we need to specify "left" and "right" here if this is a bearing?

Also, I think clockwise and anti-clockwise might be more appropriate here as they are less ambiguous.

I think the issue here is that 'bearing' is the wrong variable name. I don't like mentioning clockwise or counter clockwise, as it's given without context here, and they might think it's the orientation of the marker

either way, this isn't changed in this PR and thus should be fixed separately

I think bearing is probably fine here, if not 100% precise. The variable holds the bearing of the marker relative to the bearing of the camera. I feel using "left/right" to indicate "to the left/right of the camera" is acceptable here.

RealOrangeOne · 2018-03-07T07:23:42Z

robot/markers.py

@@ -168,9 +168,12 @@ def spherical(self) -> SphericalCoord:
        """
        return SphericalCoord(*self._raw_data['spherical'])

+    def __repr__(self):
+        return "<robot.markers.{} >".format(str(self))


I think replacing robot.markers. with nothing will lead to a nicer output. <Marker 1: ... m away> looks quite nice

kierdavis · 2018-03-07T09:21:06Z

IMO __repr__ should aim to produce a string that could be passed back to exec to reproduce the object - if you want a pretty string that's what __str__ is for. I'm open to objections though.

Anyone know if there's a PEP for the recommended behaviour of __repr__?

Adimote · 2018-03-07T15:34:11Z

@kierdavis I tend to agree, however in this case it would be a really long string. I believe the agreed method for this is to try for something which can be passed to exec(), but if it's not possible or it produces less useful output, it's fine to do your own thing. In our case we pass radians around everywhere but it would be more intuitive for students to read if our outputs were in degrees.

trickeydan · 2018-03-07T15:36:32Z

From Python 3 docs: For many types, this function(__repr__) makes an attempt to return a string that would yield an object with the same value when passed to eval(), otherwise the representation is a string enclosed in angle brackets that contains the name of the type of the object together with additional information often including the name and address of the object.

Adimote · 2018-03-07T15:38:42Z

One thing I do think should always happen though is the output of __repr__ should be unique for the reading. I don't think this means we need to give a hash of the object, but it should produce accurate enough numbers for slightly different readings to output a different __repr__

PeterJCLaw · 2018-03-07T21:33:05Z

robot/board.py

@@ -155,8 +155,11 @@ def close(self):
        """
        self.socket.detach()

+    def __repr__(self):
+        return "<{}>".format(str(self))


Please drop redundant str call here.

>>> class Foo: ... def __repr__(self): ... return 'repr' ... def __str__(self): ... return 'str' ... >>> print('{}'.format(Foo())) str

PeterJCLaw · 2018-03-07T21:33:28Z

robot/markers.py

@@ -168,9 +168,12 @@ def spherical(self) -> SphericalCoord:
        """
        return SphericalCoord(*self._raw_data['spherical'])

+    def __repr__(self):
+        return "<{}>".format(str(self))


Please drop redundant str call here (as above).

half-fixed (& broken something unrelated) in 7f9cfee
Then undid the stupid breakage in 515e0a3 (not sure what I was on when I thought I fixed that.)
Then the other half is fixed in d4e364e

please re-review, have fixed the suggested changes (after 22 days lol)

PeterJCLaw

The changes look ok, though the premise of this PR is wrong -- print(x) calls __str__ not __repr__:

In [1]: class Foo:
   ...:     def __repr__(self):
   ...:         return 'repr(Foo)'
   ...:     def __str__(self):
   ...:         return 'str(Foo)'
   ...:     

In [2]: print(Foo())
str(Foo)

Note that the default __str__ calls __repr__, which may have been the source of this confusion.

I would also rather that our __repr__s behaved more like they're meant to. For the markers, I think having an abstract description is OK, though for the boards I think we should probably do something more accurate (see inline).

PeterJCLaw · 2018-03-30T20:30:27Z

robot/board.py

@@ -155,8 +155,11 @@ def close(self):
        """
        self.socket.detach()

+    def __repr__(self):
+        return "<{}>".format(self)


I suggest that this be:

return '{}({!r})'.format(self.__class__.__name__, self.serial)

That would be truer repr behaviour, though would admittedly rely on classes which take other construction parameters overriding the default.

If we do that, we can then drop the __str__ entirely.

I disagree, this would not be truer repr behaviour. Please see my above comment and link to the Python 3 documentation.

Ah. Apparently I meant self.socket_path, not self.serial.

In which case we might want to keep the __str__.

PeterJCLaw · 2018-03-31T23:34:01Z

@Adimote was there a particular use-case or specific instance where you had seen a non-useful stringified board instance which this was trying to fix?

If not, I suggest we shelve this until after the competition.

add __repr__ for Board class

5b5b002

Adimote requested review from PeterJCLaw, RealOrangeOne and trickeydan March 6, 2018 22:49

add marker __repr__ functions, make __str__ formal

2e6e65d

PeterJCLaw suggested changes Mar 7, 2018

View reviewed changes

Adimote added 2 commits March 7, 2018 01:00

fix tests

99cb3b3

call str() instead of __str__()

ac2266c

Adimote force-pushed the fix_reprs branch from f6b302c to ac2266c Compare March 7, 2018 01:01

trickeydan reviewed Mar 7, 2018

View reviewed changes

RealOrangeOne reviewed Mar 7, 2018

View reviewed changes

remove 'robot.markers' from Markers __repr__

8b485a5

use __class__ to properly get classnames

dada805

PeterJCLaw previously requested changes Mar 7, 2018

View reviewed changes

Adimote added 3 commits March 9, 2018 13:06

remove redundant str calls

7f9cfee

undo mistake

515e0a3

remove redundant str call in board

d4e364e

Adimote requested a review from PeterJCLaw March 29, 2018 22:57

PeterJCLaw reviewed Mar 30, 2018

View reviewed changes

add __repr__ for Board and Markers #86

Are you sure you want to change the base?

add __repr__ for Board and Markers #86

Uh oh!

Conversation

Adimote commented Mar 6, 2018

Uh oh!

PeterJCLaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kierdavis commented Mar 7, 2018

Uh oh!

Adimote commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trickeydan commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adimote commented Mar 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterJCLaw left a comment

Choose a reason for hiding this comment

Uh oh!

PeterJCLaw Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterJCLaw Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterJCLaw commented Mar 31, 2018

Uh oh!

Uh oh!

add repr for Board and Markers #86

add repr for Board and Markers #86

Adimote commented Mar 7, 2018 •

edited

Loading

trickeydan commented Mar 7, 2018 •

edited

Loading

PeterJCLaw Mar 30, 2018 •

edited

Loading

PeterJCLaw Mar 31, 2018 •

edited

Loading