Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

add __repr__ for Board and Markers #86

Open
Adimote wants to merge 9 commits into
base: master
Choose a base branch
from
Open

add __repr__ for Board and Markers #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5b5b002
add __repr__ for Board class
Adimote Mar 6, 2018
2e6e65d
add marker __repr__ functions, make __str__ formal
Adimote Mar 6, 2018
99cb3b3
fix tests
Adimote Mar 7, 2018
ac2266c
call str() instead of __str__()
Adimote Mar 7, 2018
8b485a5
remove 'robot.markers' from Markers __repr__
Adimote Mar 7, 2018
dada805
use __class__ to properly get classnames
Adimote Mar 7, 2018
7f9cfee
remove redundant str calls
Adimote Mar 9, 2018
515e0a3
undo mistake
Adimote Mar 29, 2018
d4e364e
remove redundant str call in board
Adimote Mar 29, 2018
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
5 changes: 4 additions & 1 deletion robot/board.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,11 @@ def close(self):
"""
self.socket.detach()

def __repr__(self):
return "<{}>".format(self)
Copy link
Contributor

@PeterJCLaw PeterJCLaw Mar 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@trickeydan trickeydan Mar 31, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@PeterJCLaw PeterJCLaw Mar 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@PeterJCLaw PeterJCLaw Mar 31, 2018

Choose a reason for hiding this comment

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

In which case we might want to keep the __str__.


def __str__(self):
return "{} - {}".format(self.__name__, self.serial)
return "{} - {}".format(self.__class__.__name__, self.serial)

__del__ = close

Expand Down
5 changes: 4 additions & 1 deletion robot/markers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,12 @@ def spherical(self) -> SphericalCoord:
"""
return SphericalCoord(*self._raw_data['spherical'])

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

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",
Copy link
Contributor

@trickeydan trickeydan Mar 7, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Adimote Adimote Mar 7, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Adimote Adimote Mar 7, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@kierdavis kierdavis Mar 7, 2018

Choose a reason for hiding this comment

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

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.

Expand Down
4 changes: 2 additions & 2 deletions tests/test_markers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def test_str_left(self):
}
self.assertEqual(
str(Marker(data)),
"<Marker 13: 12° left, 0.12m away>",
"Marker 13: 12° left, 0.12m away",
)

def test_str_right(self):
Expand All @@ -51,5 +51,5 @@ def test_str_right(self):
}
self.assertEqual(
str(Marker(data)),
"<Marker 13: 12° right, 0.12m away>",
"Marker 13: 12° right, 0.12m away",
)