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.

Sign up for GitHub

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
+57 −12
Draft

CI: add mypy to strict type check #3165

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
126b322
CI: add mypy to strict type check
beauxq Apr 17, 2024
ae0ae60
ooops
beauxq Apr 17, 2024
2c159e5
Merge branch 'main' into add-mypy-to-ci
beauxq Apr 20, 2024
0e4b08e
Apply suggestions from code review
beauxq Apr 20, 2024
fd6da38
Merge branch 'main' into add-mypy-to-ci
beauxq May 2, 2024
6c065be
bump versions
beauxq Jul 2, 2024
9a16d45
Merge branch 'main' into add-mypy-to-ci
beauxq Jul 2, 2024
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
View file
Open in desktop
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 pyright() -> int:
""" returns process return code """
beauxq marked this conversation as resolved.
Show resolved Hide resolved
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 mypy() -> int:
""" returns process return code """
beauxq marked this conversation as resolved.
Show resolved Hide resolved
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 •
edited
Loading

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 •
edited
Loading

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 = mypy()
pyright_ret = pyright()
beauxq marked this conversation as resolved.
Show resolved Hide resolved
exit(mypy_ret and pyright_ret)
beauxq marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 3 additions & 3 deletions .github/workflows/strict-type-check.yml
View file
Open in desktop
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.358 mypy==1.9.0
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