Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: add mypy to strict type check #3165

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 54 additions & 9 deletions .github/type_check.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,60 @@
import json
from pathlib import Path
import subprocess
from typing import Any, Dict, List, Union

config = Path(__file__).parent / "pyright-config.json"
source_dir = Path(__file__).parent
config = source_dir / "pyright-config.json"

command = ("pyright", "-p", str(config))
print(" ".join(command))

try:
result = subprocess.run(command)
except FileNotFoundError as e:
print(f"{e} - Is pyright installed?")
exit(1)
def run_pyright() -> int:
""" returns process exit code """
command = ("pyright", "-p", str(config))
print(" ".join(command))

exit(result.returncode)
try:
pyright_result = subprocess.run(command)
except FileNotFoundError as e:
print(f"{e} - Is pyright installed?")
exit(1)
return pyright_result.returncode


def run_mypy() -> int:
""" returns process exit code """
with open(config) as config_file:
config_data: Union[Dict[str, Any], Any] = json.load(config_file)
Comment on lines +25 to +26
Copy link
Member

@black-sliver black-sliver Apr 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't mypy use a separate list of files, so as the maintainer of a file they can decide which one they think should be applied for their code (assuming they are not 1:1 compatible)?

Copy link
Collaborator Author

@beauxq beauxq Apr 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't really isolate the checks to a specific list of files.
The list of files we give (to either mypy or pyright) is not a list of files to check, It's a list of files to report errors in. Both type checkers follow imports to get type information from other files. So the type information in any file is going to affect both checks.
If someone thinks they're only using mypy, because the file they're changing is only on the mypy list, that change will still affect the pyright check, even though the file is not on the pyright list.

Because we can't completely isolate them, I don't think it's worth it to maintain 2 separate lists.

The main focus of this is core, and worlds are opt-in.
If this PR is accepted (I don't consider it decided yet whether we want this), If a world maintainer wants to only use one, then they shouldn't opt in to this system, and should just check with their own workflow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't think of this as a linter, checking stylistic things.
You should think of it as a static type checker, just like in a C++ or Rust compiler.

I think it wouldn't make sense to say:
"For this C++ file x.cpp I only care whether clang's type checker passes, but for y.cpp I only care whether gcc's type checker passes."
We would care about everything passing, in one compiler, or the other compiler, or we want to support both compilers.

I think it would be good to support multiple static type checkers, just like a C++ project that wants to support multiple compilers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That argument makes sense, yeah.

That being said, I honestly don't know how we (or who) decide(s) if we want to merge this or not.


assert isinstance(config_data, dict)
file_list: Union[List[str], None, Any] = config_data.get("include")
assert isinstance(file_list, list), f"unknown data in config file: {type(file_list)=}"
file_list = [
str(source_dir / file_name)
for file_name in file_list
]
params = [
"mypy",
"--strict",
"--follow-imports=silent",
"--no-warn-unused-ignore",
"--install-types",
"--non-interactive",
"typings",
]

command = params + file_list
print(" ".join(params))

try:
mypy_result = subprocess.run(command)
except FileNotFoundError as e:
print(f"{e} - Is mypy installed?")
exit(1)
return mypy_result.returncode


if __name__ == "__main__":
# mypy is first because of its --install-types feature
mypy_ret = run_mypy()
pyright_ret = run_pyright()
exit(mypy_ret or pyright_ret)
6 changes: 3 additions & 3 deletions .github/workflows/strict-type-check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ on:
- "**.pyi"

jobs:
pyright:
type-check:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
Expand All @@ -26,8 +26,8 @@ jobs:

- name: "Install dependencies"
run: |
python -m pip install --upgrade pip pyright==1.1.358
python -m pip install --upgrade pip pyright==1.1.369 mypy==1.10.1
python ModuleUpdate.py --append "WebHostLib/requirements.txt" --force --yes

- name: "pyright: strict check on specific files"
- name: "type check: strict check on specific files"
run: python .github/type_check.py
Loading