feat: Add mypy type checking #4863

zachaysan · 2024-11-25T16:55:04Z

Changes

This adds mypy type checking. Existing failures have been grandfathered in and a helper script is run to compare failures against the predefined list. Mypy was also added into the pre-commit config and will run on any commits to python code.

How did you test this code?

I altered the predefined mypy list and saw a failure crop up when running pre-commit.

vercel · 2024-11-25T16:55:08Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
docs	⬜️ Ignored (Inspect)	Visit Preview	Dec 9, 2024 8:05pm
flagsmith-frontend-preview	⬜️ Ignored (Inspect)	Visit Preview	Dec 9, 2024 8:05pm
flagsmith-frontend-staging	⬜️ Ignored (Inspect)	Visit Preview	Dec 9, 2024 8:05pm

github-actions · 2024-11-25T16:55:30Z

Docker builds report

Image	Build Status	Security report
`ghcr.io/flagsmith/flagsmith-api-test:pr-4863`	Finished ✅	Skipped
`ghcr.io/flagsmith/flagsmith-e2e:pr-4863`	Finished ✅	Skipped
`ghcr.io/flagsmith/flagsmith-frontend:pr-4863`	Finished ✅	Results ✅
`ghcr.io/flagsmith/flagsmith-api:pr-4863`	Finished ✅	Results ✅
`ghcr.io/flagsmith/flagsmith:pr-4863`	Finished ✅	Results ✅
`ghcr.io/flagsmith/flagsmith-private-cloud:pr-4863`	Finished ✅	Results ✅

github-actions · 2024-11-25T16:59:54Z

Uffizzi Ephemeral Environment `deployment-58551`

☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/4863

📄 View Application Logs etc.

What is Uffizzi? Learn more!

codecov · 2024-11-25T17:07:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.38%. Comparing base (05ab7cf) to head (c9b44d4).
Report is 14 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4863   +/-   ##
=======================================
  Coverage   97.38%   97.38%           
=======================================
  Files        1189     1189           
  Lines       41420    41453   +33     
=======================================
+ Hits        40338    40371   +33     
  Misses       1082     1082

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

matthewelwell

This seems like a decent enough solution to me but I have some general questions:

What happens if we (perhaps inadvertently) resolve an existing issue. Based on the logic in your script, I guess we wouldn't know about it, right? Because we're just seeing what's left after we subtract the original errors from the new ones which wouldn't tell us anything about any that we've solved. It would obviously be great to incentivise resolving existing issues where we can.
This approach works fine I guess, but another solution would have been to just add all of the distinct files into the exclude. Then every new file would need to conform to mypy, but changes to existing files wouldn't. I'm not sure how we could incentivise fixing the existing errors. (Note: I'm certainly not suggesting this is a better option, I'm just playing devil's advocate).

matthewelwell · 2024-11-27T10:54:41Z

.pre-commit-config.yaml

+    hooks:
+      - id: mypy-check
+        name: mypy check
+        entry: bash -c "cd api && poetry run python scripts/mypy_check.py"


Since I was looking at similar functionality yesterday (see here) , I am somewhat intrigued by this.

My understanding of pre-commit is that it passes the modified files into the command specified in entry, so the resulting command would look something like:

bash -c "cd api && poetry run python scripts/mypy_check.py" file1.py file2.py

... which I would expect to just not work at all, or at least throw some sort of error?

@zachaysan I'm not sure the latest changes make any reference to this?

I've altered the script to run the files one at a time. I had to use git diff to list the target files of the commit because no matter what I tried to pass into the bash script it just wouldn't run with the automatic listed files. The good news is that we are now doing a mypy check on only the committed files.

api/mypy.ini

zachaysan · 2024-12-04T16:11:26Z

This seems like a decent enough solution to me but I have some general questions:

What happens if we (perhaps inadvertently) resolve an existing issue. Based on the logic in your script, I guess we wouldn't know about it, right? Because we're just seeing what's left after we subtract the original errors from the new ones which wouldn't tell us anything about any that we've solved. It would obviously be great to incentivise resolving existing issues where we can.

I think we should treat the baseline as something we can devote engineering time towards. Delete a chunk of the baseline then re-run mypy see the failures that you eliminated from the baseline then handle them. Over time, I think we can get to a point where the baseline file is deleted and we have full coverage.

This approach works fine I guess, but another solution would have been to just add all of the distinct files into the exclude. Then every new file would need to conform to mypy, but changes to existing files wouldn't. I'm not sure how we could incentivise fixing the existing errors. (Note: I'm certainly not suggesting this is a better option, I'm just playing devil's advocate).

I think the approach I've taken is actually better for coverage because new errors in existing files will be surfaced. As for incentivizing fixing the existing errors, I really think we should just have engineering time to delete things from the baseline then fix them up locally.

matthewelwell

Approving with a minor comment, but as discussed in the engineering meeting, I'd like to get everyone's eyes on this.

api/scripts/mypy_check.py

khvn26 · 2024-12-06T17:26:55Z

pre-commit.ci run

khvn26 · 2024-12-06T14:09:25Z

api/pyproject.toml

@@ -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/)$"


Is there a reason to exclude the test code?

Mypy throws an error when we try to include the tests module in the coverage. We tried hard to figure out how to solve this bug but it was outside of our grasp.

khvn26 · 2024-12-06T16:45:30Z

api/scripts/mypy_check.py

+
+    # 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}
+    new_errors = current_errors - baseline


I think we should check for baseline - current_errors as well to make sure the baseline file contains up to date errors. Rarely, but things may change implicitly (e.g. with a dependency update).

Great idea. I've implemented this with a helpful error message.

Just realized that this doesn't actually work since mypy only checks errors for the current working module. I've reverted my change.

khvn26 · 2024-12-06T16:46:48Z

api/scripts/mypy_check.py

+        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}


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?

Since the line numbers change between installs it becomes difficult to handle it that way. Plus, site-packages is going to be only ever present in the installation path for non-local modules.

khvn26 · 2024-12-06T17:35:36Z

.pre-commit-config.yaml

+    hooks:
+      - id: mypy-check
+        name: mypy check
+        entry: bash -c "cd api && poetry run python scripts/mypy_check.py $(git diff --name-only --cached -- '*.py')"


As seen from pre-commit.ci run here, this hook won't work when poetry isn't present globally.

Maybe try adding

additional_dependencies: ['poetry']

or using repo: local (see docs here).

If all else fails, moving the hook to its own repo could be an option.

The additional_dependencies seems to have worked once I changed the language to python.

matthewelwell · 2024-12-09T11:57:56Z

pre-commit.ci run

@zachaysan I think you'll need to rebase onto the main branch to get pre-commit to pass after I merged this PR.

…ery module

zachaysan added 7 commits November 25, 2024 16:41

Add mypy type checking to pre-commit

4eb8e56

Add mypy to pyproject and update necessary dependencies

e92c2ef

Create mypy.ini

1b553f0

Add __init__.py files for mypy

83aebeb

Fix failing lint

8abf743

Add mypy baseline failures

359317e

Add mypy checking script

319a14e

zachaysan requested a review from a team as a code owner November 25, 2024 16:55

zachaysan requested review from matthewelwell and removed request for a team November 25, 2024 16:55

github-actions bot added the api Issue related to the REST API label Nov 25, 2024

github-actions bot added the feature New feature or request label Nov 25, 2024

Move mypy to the end since it is the slowest

a9fa1cd

github-actions bot added feature New feature or request and removed feature New feature or request labels Nov 25, 2024

matthewelwell reviewed Nov 27, 2024

View reviewed changes

matthewelwell linked an issue Dec 2, 2024 that may be closed by this pull request

Enforcing typing #4708

Open

zachaysan added 5 commits December 3, 2024 16:41

Delete mypy.ini

a410134

Add mypy concerns to pyproject.toml

b7c4974

Update mypy checking

1fa8173

Add type ignores

91e79f3

Fix conflicts and merge branch 'main' into feat/add_mypy_type_checking

dbe12f9

github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 3, 2024

vercel bot deployed to Preview – docs December 3, 2024 20:28 View deployment

vercel bot deployed to Preview – flagsmith-frontend-preview December 3, 2024 20:30 View deployment

zachaysan added 3 commits December 4, 2024 15:43

Fix conflicts and merge branch 'main' into feat/add_mypy_type_checking

45b6f95

Update mypy_baseline

d572a36

Filter out mypy failures for site packages

2e79ae8

github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 4, 2024

zachaysan added 2 commits December 4, 2024 16:03

Add errors to baseline

003c067

Update baseline and revert changes to util model type ignores

57f674d

github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 4, 2024

zachaysan requested a review from matthewelwell December 4, 2024 16:11

matthewelwell approved these changes Dec 5, 2024

View reviewed changes

api/scripts/mypy_check.py Outdated Show resolved Hide resolved

Remove command from script output

57bab41

github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 5, 2024

zachaysan requested review from khvn26 and gagantrivedi December 5, 2024 20:11

gagantrivedi approved these changes Dec 6, 2024

View reviewed changes

khvn26 requested changes Dec 6, 2024

View reviewed changes

zachaysan added 3 commits December 9, 2024 19:50

Add baseline updating to mypy check script

8895a11

Fix conflicts and merge branch 'main' into feat/add_mypy_type_checking

5ca6332

Try setting up mypy check for CI

408e9ad

github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 9, 2024

Revert now acceptable check since mypy doesn't generate errors for ev…

c9b44d4

…ery module

github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 9, 2024

zachaysan requested a review from khvn26 December 9, 2024 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add mypy type checking #4863

feat: Add mypy type checking #4863

zachaysan commented Nov 25, 2024

vercel bot commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

matthewelwell left a comment

matthewelwell Nov 27, 2024

matthewelwell Dec 4, 2024

zachaysan Dec 4, 2024

zachaysan commented Dec 4, 2024

matthewelwell left a comment

khvn26 commented Dec 6, 2024

khvn26 Dec 6, 2024

zachaysan Dec 9, 2024

khvn26 Dec 6, 2024

zachaysan Dec 9, 2024

zachaysan Dec 9, 2024

khvn26 Dec 6, 2024

zachaysan Dec 9, 2024

khvn26 Dec 6, 2024

zachaysan Dec 9, 2024

matthewelwell commented Dec 9, 2024

feat: Add mypy type checking #4863

Are you sure you want to change the base?

feat: Add mypy type checking #4863

Conversation

zachaysan commented Nov 25, 2024

Changes

How did you test this code?

vercel bot commented Nov 25, 2024 • edited Loading

github-actions bot commented Nov 25, 2024 • edited Loading

Docker builds report

github-actions bot commented Nov 25, 2024 • edited Loading

Uffizzi Ephemeral Environment deployment-58551

codecov bot commented Nov 25, 2024 • edited Loading

Codecov Report

matthewelwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachaysan commented Dec 4, 2024

matthewelwell left a comment

Choose a reason for hiding this comment

khvn26 commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewelwell commented Dec 9, 2024

vercel bot commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

Uffizzi Ephemeral Environment `deployment-58551`

codecov bot commented Nov 25, 2024 •

edited

Loading