Introduce SpriteSequence, a covariant supertype of SpriteList.#2647
Introduce SpriteSequence, a covariant supertype of SpriteList.#2647pushfoo merged 6 commits intopythonarcade:developmentfrom
Conversation
sjrd
force-pushed
the
sprite-sequence
branch
2 times, most recently
from
4890b9f to
b11fb9a
Compare
@DragonMoffon and I want to change |
There was a problem hiding this comment.
Thank you for starting taking a crack at this. I appreciate it since it's sorely needed (see #2580 and similar 😅).
For the moment, I'm a little confused on some abc-related details as in this comment. If nobody beats me to giving this a closer look, I may need a few days before I can give it the full attention it deserves.
@eruvanos btw, do you think we should add a typing-specific tests folder or similar as an exception to the tests folder being ignored?
|
@pushfoo May I suggest a ping here? Is there anything else you'd like me to do? Thank you. |
|
@pushfoo Regarding the typed test folder: if we have tests that benefit from such a setup I am fine, I wonder if it makes our test structure more complicated. Regarding Scene and TileMap: we can iterate on them after we merge this PR. I just created a small code base over easter which uses Scene and TileMap, so I could doublecheck the improved versions. @sjrd we do not have a strict squashing policy, I prefer squashed PRs to reduce the amount of commits we have to read through during write up of release notes. |
As we are just out of a while loop whose stopping condition is that `sprite_lists` is empty, calling `clear()` on it is redundant.
There was already a method `register_sprite_list` to handle all additions to `sprite_lists`. We add a corresponding method `_unregister_sprite_list` to handle removals. We make the typings of these methods stricter, so that we can enforce the correct typing invariant on `sprite_lists`. We also make that invariant clearer in a comment. `sprite_lists` is unfortunately unsafely visible to everyone. So a user of the class could still violate the invariants. At least now the *intended* usage is safe.
This is similar to the fix done to `check_for_collision_with_list` done in c387717.
Adding type parameters to some `SpriteList`s. One allows to get rid of a cast.
|
Thanks. I squashed the last two commits, and added the entry to the changelog. I had to rebase on top of #2652 not to have conflicts. The only changes besides rebasing are the |
This is done by analogy to `collections.abc.Sequence`, which is a covariant supertype of `list`. Before this commit, many parts of the codebase used `SpriteList`s without type arguments (defaulting to `Unknown`). That was the only way to allow reasonable usages of the given methods and attributes. However, doing so results in weaker typing. Using `SpriteSequence`, we can add correct type arguments to almost all of the references that were using `SpriteList`s before. The only missing pieces are `Scene` and `TileMap`. Unfortunately, their APIs are fundamentally unsound wrt. the type arguments of their `SpriteList`s. We cannot make it sound without breaking their APIs, so we do not change them. As a bonus, we can now create lists of `SpriteList`s with varying type arguments, and generically call `draw` or `update` on them. Previously, the only common supertype of `SpriteList[A]` and `SpriteList[B]` was `object`, which meant it was not possible to call those methods on them. In a sense, that ability mostly subsumes the convenience provided by `Scene`. A `list[SpriteSequence[BasicSprite]]` is almost as convenient, while being type-safe.
|
@pushfoo are you fine with the changes? I think we could move on with this and iterate on Scene and Tilemap (and typed tests afterwards). What is your opinion? |
pushfoo
left a comment
pushfoo
left a comment
There was a problem hiding this comment.
Sorry for the delay. I'll say let's merge it to keep velocity and revisit if we need to. Ty for your contribution and your patience.
3 participants
This is done by analogy to
collections.abc.Sequence, which is a covariant supertype oflist.Before this commit, many parts of the codebase used
SpriteLists without type arguments (defaulting toUnknown). That was the only way to allow reasonable usages of the given methods and attributes. However, doing so results in weaker typing.Using
SpriteSequence, we can add correct type arguments to almost all of the references that were usingSpriteLists before.The only missing pieces are
SceneandTileMap. Unfortunately, their APIs are fundamentally unsound wrt. the type arguments of theirSpriteLists. We cannot make it sound without breaking their APIs, so we do not change them.As a bonus, we can now create lists of
SpriteLists with varying type arguments, and generically calldraworupdateon them. Previously, the only common supertype ofSpriteList[A]andSpriteList[B]wasobject, which meant it was not possible to call those methods on them.In a sense, that ability mostly subsumes the convenience provided by
Scene. Alist[SpriteSequence[BasicSprite]]is almost as convenient, while being type-safe.This was prompted by seeing students struggle with the absence of a common supertype for
SpriteList[A]andSpriteList[B]. Students resorted tocasts orAnys (or smuggled their way out of that usingScene) to work around the issue. WithSpriteSequence, this shouldn't be an issue anymore.It turns out that the introduction of
SpriteSequenceallows to more safely type many APIs of Arcade, so hopefully this addition carries its weight.