search

sourcebots / robot-api

(Legacy) API to interface with robotd
http://docs.sourcebots.co.uk/api/
MIT License
4 stars 1 forks source link

Add some tests to pin down the behaviour of BoardList #69

Open PeterJCLaw opened 6 years ago

PeterJCLaw commented 6 years ago

These currently fail as I'm not sure what the expected behaviours are.

RealOrangeOne commented 6 years ago

The idea of BoardList is that it's a list of boards, which can be indexed by both the serial number of the board, and an integer index. When using an integer index, the order is based off the serial number, to be deterministic

PeterJCLaw commented 6 years ago

@RealOrangeOne I think the issue for me is that while it's called a list, and somewhat mostly behaves like a list, we've marked it as a Mapping rather than a Sequence and it's construction is around a dictionary of boards. I'm not sure how much of this has been thought through with regards to the usual rules for sequences versus maps and how much has just sort-of happened.

At the moment it's in this odd state where some of the usual operations work while others don't. In particular I spotted that 0 in BoardList() raises IndexError, which is definitely wrong:

>>> 0 in BoardList()  # Should definitely return False
IndexError  # wut
>>> 0 in BoardList({'x': FakeBoard('x'))  # Mappings contain their keys, not their values (sequences contain their elements)
True
>>> [x for x in BoardList({'x': FakeBoard('x'))]  # Sequences iterate over their elements (mappings iterate over their keys)
[FakeBoard('x')]

For clarity: I know why these behaviours are happening given the current state of the code, the bit I'm not sure about is what we want the behaviours to be.

RealOrangeOne commented 6 years ago

The lower 2 examples here are as expected, that functionality is fine. Unfortunately there's no way of changing in so it only counts the serial numbers rather than index, but that's a minor annoyance.

I believe the issue with the IndexError is in __getitem__, which doesnt look to safely get the properties from the internal data. Perhaps looking into how in works, and adjusting the method accordingly is the best course of action

PeterJCLaw commented 6 years ago

@RealOrangeOne I disagree -- if this list is going to iterate over the boards it contains, then it should also claim only to contain those items (and not the indexes which point at them).

(You're correct about the cause of the IndexError; the fix for all of this is to implement a __contains__ method; my question is about the overall behaviour we want)