Simplify board lists #79
Conversation
As far as I can tell, nothing relies on these being sorted.
This simplifies the logic in Robot and makes the construction interface to BoardList much more list-like.
This changes _update_boards so that it modifies the lists of boards in-place rather than returning them and has it instead return a `BoardList[T]` of all boards. This allows us to reduce the logic in the accessor properties about as far as is possible. Remove the TODO tag now that we've reduced the boards' properties to their realistic minimum, even though we've not quite achieved what it suggests.
| @@ -57,89 +59,79 @@ def _assert_has_power_board(self): | |||
| if not power_boards: | |||
| raise RuntimeError('Cannot find Power Board!') | |||
|
|
|||
| def _update_boards(self, known_boards, board_type, directory_name): | |||
| def _update_boards( | |||
There was a problem hiding this comment.
Perhaps taking typing too far, but it'd be nice to use a type variable for this over any TBoard. Therefore we can specify that the known boards, the board type, and the resulting board list are all of the same type
There was a problem hiding this comment.
I'm not sure I follow -- my understanding is that that's what this does?
Modifying a usage to show the breakage:
@property
def motor_boards(self) -> BoardList[MotorBoard]:
"""
:return: A ``BoardList`` of available ``MotorBoard``s.
"""
return self._update_boards(self.known_motor_boards, PowerBoard, 'motor')
# [mypy] error:Argument 2 to "_update_boards" of "Robot" has incompatible type "Type[PowerBoard]"; expected "Type[MotorBoard]"There was a problem hiding this comment.
Thinking about this a bit more, are you suggesting that it end up looking a bit more like this:
# wrapper declared to show the change, obviously we'd actually
# collapse the two together.
def _update_boards2(self, board_type: Type[TBoard]) -> BoardList[TBoard]:
return self._update_boards(
self.known_boards[board_type],
board_type,
board_type.directory,
)If so, I actually wondered about something like that.
The issue is that there doens't seem to be a way to specify the type of the known_boards dict which it would need. In theory it would be something like Dict[Type[TBoard], List[TBoard]], however that doesn't work like you'd hope it would -- the TBoard needs to be a single value for the whole type, rather than being dynamic over the key/value pairs in the dict.
I think you could do something a bit like that with either a function in the place of the known_boards mapping or maybe a custom type, though either solution ends up adding more code than we'd really want for the resulting gain. If we were doing a lot of operations like that then it might be worth it, but I don't think we're really on that scale here.
I do think there's room to have the directories specified on the types (which I've only just spotted), though I think that's outside the scope of this PR.
There was a problem hiding this comment.
TBoard creates a link to check that it's a valid board type being used in all the right locations, but it doesn't make sure all those boards are of the same type. I don't think anyway?
There was a problem hiding this comment.
TBoard ensures that all the arguments to _update_board are of the same kind of Board. Its return type then couples that to being the right type for the property.
I'm still not sure I understand what your concern/suggestion is here -- could you provide an example (probably code/psuedocode) of either a broken scenario you're trying to prevent, or of what usage of the better API might look like?
There was a problem hiding this comment.
By that statement, this is probably fine 👍
This changes
_update_boardsso that it handles all the logic needed by the various boards properties and resolves the TODO around that. It also moves more of the logic around the construction ofBoardListinto that class, making it simpler to create and to add types for.