Archipelago icon indicating copy to clipboard operation
Archipelago copied to clipboard

CI: add mypy to strict type check

Open beauxq opened this issue 1 year ago • 3 comments

What is this fixing or adding?

This is an addition to https://github.com/ArchipelagoMW/Archipelago/pull/3121 with some previous discussion here https://github.com/ArchipelagoMW/Archipelago/pull/3107

There are some errors that mypy catches and pyright doesn't, and some errors that pyright catches and mypy doesn't. So we can catch more problems if we check with both.

The one Python script .github/type_check.py will show output from both mypy and pyright.

One of the biggest issues we've found with mypy in AP is this bug: https://github.com/python/mypy/issues/10506 After lots of battling with it in https://github.com/ArchipelagoMW/Archipelago/pull/2173 and https://github.com/ArchipelagoMW/Archipelago/pull/2899, I think we worked around it so it only needs a # type: ignore for default = "random" I think that will only happen with a few options in worlds, and this check is opt-in from world maintainers.

How was this tested?

submitting this PR, because thar's the only way to test github actions

beauxq avatar Apr 17 '24 01:04 beauxq

A minor issue I found is this:

def foo(option_class: Union[Type[NumericOption], Type[FreeText]]) -> None:
    print(option_class.name_lookup.values())

reports: "error: Access to generic class variables is ambiguous [misc]" This should be pretty rare, because it only happens with the union of multiple options, and not with 1 base type Type[Option[int]]

beauxq avatar Apr 17 '24 01:04 beauxq

I guess I would like for more people to chime in and say if they think this is good or bad. It's hard for me to decide if this is an upgrade or not.

In my mind, having both allows more proper placement of typing or # type: ignore to ensure compatibility with both mypy and pyright, however pinning both in CI will also give more opportunities for it to be out of line with what people use locally.

In other words, if the CI succeeds we can be fairly certain that running any type checker locally will work, but if we get a fix in either one, we still can not "use" it until it's fixed in both and the pins got updated.

black-sliver avatar May 02 '24 06:05 black-sliver

@NewSoupVi do you have an opinion on this?

black-sliver avatar Jul 04 '24 20:07 black-sliver