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

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

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

Conversation

PeterJCLaw
Copy link
Contributor

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

These currently fail as I'm not sure what the expected behaviours are.
@RealOrangeOne
Copy link
Member

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
Copy link
Contributor Author

PeterJCLaw commented Feb 9, 2018

@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
Copy link
Member

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
Copy link
Contributor Author

@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)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants