benmoran56 / esper

An ECS (Entity Component System) for Python
MIT License
544 stars 69 forks source link

mypy --strict #85

Closed Felecarpp closed 1 year ago

Felecarpp commented 1 year ago

mypy config dotfile with strict=True full typing of esper/_init__.py World(timed=True) become TimedWorld() 3 lines uses # type : ignore to disable type check on the line

benmoran56 commented 1 year ago

Thanks for working on this.

I'm personally OK with ignoring some things for now, and revisiting them in the future. I guess the arguments are maybe more important than the return types (if only by a little), so getting those defined is a nice improvement. How about merging this in for now?

As for typing in general, whenever older Python releases go EOL it's a good time to see what syntax can be refined. I'm personally very much looking forward to using | for Union in the future ๐Ÿ˜„

Felecarpp commented 1 year ago

Ok to merge as it is.

The X | Y for Union syntax is still recent (python 3.10). Using it means to take two python releases off at the time (python 3.8 and python 3.9).

Another typing improvement (python 3.9) is using subscription on builtin types like list[tuple[str, int]] (so remove some imports).

gretkierewicz commented 1 year ago

Thanks for working on this.

I'm personally OK with ignoring some things for now, and revisiting them in the future. I guess the arguments are maybe more important than the return types (if only by a little), so getting those defined is a nice improvement. How about merging this in for now?

As for typing in general, whenever older Python releases go EOL it's a good time to see what syntax can be refined. I'm personally very much looking forward to using | for Union in the future ๐Ÿ˜„

If so, @Felecarpp please create issue first not to forget about removing those ignores. But as far as I know cast is just for static typing so not affecting performance.

Felecarpp commented 1 year ago

https://docs.python.org/3/library/typing.html#typing.cast

at runtime we intentionally donโ€™t check anything (we want this to be as fast as possible)

So yes, typing.cast do not affect performance significantly.

HexDecimal commented 1 year ago

The X | Y for Union syntax is still recent (python 3.10). Using it means to take two python releases off at the time (python 3.8 and python 3.9).

Another typing improvement (python 3.9) is using subscription on builtin types like list[tuple[str, int]] (so remove some imports).

With from __future__ import annotations both of these features can be used since Python 3.7.

typing.cast isn't no cost. It costs less then isinstance but still has the overhead of a Python call. It depends on how often the cast is called but I'd try to keep it out of the insertion, removal, or access of components. Personally, I wouldn't use the casts and instead change # type: ignore to add the specific error code # type: ignore[no-any-return].

Felecarpp commented 1 year ago

About X | Y, an other PR can add this syntax.

About typing.cast, I agree @HexDecimal. The minimal performance cost is using # type: ignore in the function body. The typing in function header is enough to make mypy running when the function is called. So I think to commit again about this.

benmoran56 commented 1 year ago

If you're OK, I'm happy to merge this as is. We can always follow up later.

Felecarpp commented 1 year ago

I am OK to merge this as is.

GitHub shows me "Only those with write access to this repository can merge pull requests.".