GameObjectCollection.add: Behave correctly with inheritance #241
Conversation
Otherwise, selection of objects of a given kind misses objects belonging to a subclass.
|
I had missed a necessary change in |
| @@ -48,7 +48,9 @@ def add(self, game_object: Hashable, tags: Iterable[Hashable]=()) -> None: | |||
| if isinstance(tags, (str, bytes)): | |||
| raise TypeError("You passed a string instead of an iterable, this probably isn't what you intended.\n\nTry making it a tuple.") | |||
| self.all.add(game_object) | |||
| self.kinds[type(game_object)].add(game_object) | |||
|
|
|||
| for kind in type(game_object).mro(): | |||
There was a problem hiding this comment.
This is a tricky thing: mro here is going to include BaseSprite, Sprite (the ABC), and object. Are you intending for that to happen or would you rather explicitly exclude those?
There was a problem hiding this comment.
I couldn't think of any reasons to exclude them (except maybe object?), and there are no documented exceptions about what are valid kinds.
There was a problem hiding this comment.
The question is: what's the difference between self.all and self.kinds[object] under this new functionality?
There was a problem hiding this comment.
We should improve the test for removal if we're adding this behavior. It didn't catch the change because the change is new functionality and wasn't part of the contract.
I'm glad for the improved functionality, but definitely need to update that test.
Also, I'd rather avoid using the mro in the tests themselves, so add an iterable with the types we should check for somehow.
What do you mean? The tests do not currently use
The tests are already updated; do you mean adding a test that checks that the object is gone from all kinds once removed? |
|
They don't, but that's also the "easy" way to write the |
|
Done. I'm not particularly ameObjectCollection` testsuite, but fixing it is way out of scope here. |
|
OK for merging this as-is, then? |
|
bors r+ |
241: GameObjectCollection.add: Behave correctly with inheritance r=nbraud a=nbraud - [x] Make scene tests also use a subclass of `TestPlayer`. This shows that `scene.get(kind=TestPlayer)` misses (instances of) subclasses. - [x] Fix the behaviour in `GameObjectCollection`. Done by adding to all kinds in the MRO. Possibly not the most efficient solution. - [x] Refactor `tests/scenes/get_methods`. Co-authored-by: Nicolas Braud-Santoni <nicolas@braud-santoni.eu>
Build succeeded
|
TestPlayer.This shows that
scene.get(kind=TestPlayer)misses (instances of) subclasses.GameObjectCollection.Done by adding to all kinds in the MRO. Possibly not the most efficient solution.
tests/scenes/get_methods.