-
Notifications
You must be signed in to change notification settings - Fork 406
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
feat: Add mypy type checking #4863
base: main
Are you sure you want to change the base?
Changes from 24 commits
4eb8e56
e92c2ef
1b553f0
83aebeb
8abf743
359317e
319a14e
a9fa1cd
a410134
b7c4974
1fa8173
91e79f3
dbe12f9
c2a9cf4
d63f5c1
77b9e41
7847a51
2281d5b
45b6f95
d572a36
2e79ae8
003c067
57f674d
57bab41
8895a11
5ca6332
408e9ad
c9b44d4
20388f8
fb7de2b
d0f3931
1393a65
96cf304
63b34a6
b2d7a4a
6297bce
7a36638
55165c9
fe37a4d
0de9e73
a382ad6
6f3b21d
21955b8
2bba3ae
522cd57
0d8a0d5
5c8449f
ac0b7cc
158ac2b
e2ace23
6d17eb0
c3e7907
fd7574c
264ecb7
6dcff38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,6 +99,16 @@ skip = ['migrations', '.venv', '.direnv'] | |
addopts = ['--ds=app.settings.test', '-vvvv', '-p', 'no:warnings'] | ||
console_output_style = 'count' | ||
|
||
[tool.mypy] | ||
exclude = "^(tests/)$" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason to exclude the test code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mypy throws an error when we try to include the |
||
plugins = ["mypy_django_plugin.main"] | ||
|
||
[tool.django-stubs] | ||
django_settings_module = "app.settings.local" | ||
|
||
[tool.drf-stubs] | ||
enabled = true | ||
|
||
[tool.poetry] | ||
name = "flagsmith-api" | ||
version = "2.68.0" | ||
|
@@ -161,7 +171,7 @@ django-ordered-model = "~3.4.1" | |
django-ses = "~3.5.0" | ||
django-axes = "~5.32.0" | ||
pydantic = "^2.3.0" | ||
pyngo = "~2.0.1" | ||
pyngo = "~2.2.1" | ||
flagsmith = "^3.6.0" | ||
python-gnupg = "^0.5.1" | ||
django-redis = "^5.4.0" | ||
|
@@ -219,6 +229,10 @@ requests-mock = "^1.11.0" | |
django-extensions = "^3.2.3" | ||
pdbpp = "^0.10.3" | ||
mypy-boto3-dynamodb = "^1.33.0" | ||
mypy = "^1.11.2" | ||
django-stubs = "~5.1.1" | ||
djangorestframework-stubs = "~3.14.0" | ||
boto3-stubs = "~1.35.67" | ||
|
||
[build-system] | ||
requires = ["poetry-core>=1.5.0"] | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
import re | ||
import subprocess | ||
import sys | ||
from pathlib import Path | ||
|
||
|
||
def main() -> None: | ||
baseline_path = Path("scripts/mypy_baseline.txt") | ||
if not baseline_path.exists(): | ||
print("Baseline file not found. Run mypy and create mypy_baseline.txt first.") | ||
sys.exit(1) | ||
|
||
# Run mypy and capture output | ||
files_to_check = [f"../{filename}" for filename in sys.argv[1:]] | ||
command = [ | ||
"poetry", | ||
"run", | ||
"mypy", | ||
"--config-file", | ||
"pyproject.toml", | ||
] + files_to_check | ||
|
||
result = subprocess.run( | ||
command, | ||
capture_output=True, | ||
text=True, | ||
) | ||
|
||
# Mypy found no issues for the targeted file | ||
if result.returncode == 0: | ||
print(result.stdout.strip()) | ||
sys.exit(0) | ||
|
||
# Mypy failed in some other way other than listing failing file lines | ||
if result.returncode != 1: | ||
print( | ||
f"Error running mypy with return code {result.returncode} and error:", | ||
result.stderr, | ||
) | ||
print(result.stdout.strip()) | ||
sys.exit(1) | ||
|
||
current_output = result.stdout.strip().splitlines() | ||
|
||
with open(baseline_path, "r") as f: | ||
baseline = set(line.strip() for line in f if line.strip()) | ||
|
||
# Detect new errors and remove information line filtering out third party packages | ||
current_errors = {line for line in current_output if "site-packages" not in line} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I appreciate it's going to be slightly more difficult to compare against, but why not just have third party errors in the baseline as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the line numbers change between installs it becomes difficult to handle it that way. Plus, |
||
new_errors = current_errors - baseline | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should check for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great idea. I've implemented this with a helpful error message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just realized that this doesn't actually work since |
||
pattern = r"Found (\d+) errors in (\d+) files \(checked (\d+) source files\)" | ||
removal = None | ||
|
||
for error in new_errors: | ||
has_match = re.match(pattern, error) | ||
if has_match: | ||
removal = error | ||
break | ||
|
||
if removal: | ||
new_errors.remove(removal) | ||
|
||
if new_errors: | ||
print("New mypy errors detected:") | ||
print("\n".join(new_errors)) | ||
sys.exit(1) | ||
|
||
print("No new mypy errors detected.") | ||
sys.exit(0) | ||
|
||
|
||
if __name__ == "__main__": | ||
main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As seen from pre-commit.ci run here, this hook won't work when poetry isn't present globally.
Maybe try adding
or using
repo: local
(see docs here).If all else fails, moving the hook to its own repo could be an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
additional_dependencies
seems to have worked once I changed thelanguage
topython
.