Require all Python to pass mypy unless specifically excluded #11272

callahad · 2021-11-08T10:15:44Z

This is a maximal response to #11271: instead of specifying files to include in mypy checking, all files are considered unless explicitly excluded.

We may want to just take the first commit instead, and focus only on the synapse/ directory. Or some middle ground.

For example: instead of getting this whole repo to pass Mypy in a single run, we may want to:

Explicitly exempt everything in contrib/, tests/, etc.
Check standalone scripts, schema migrations, etc. separately and individually, instead of as part of a single omnibus mypy run for the synapse module (doing so separately is encouraged by mypy, but the scripts_are_modules setting kind of works around it).

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
Pull request includes a sign off
Code style is correct
(run the linters)

Signed-off-by: Dan Callahan <danc@element.io>

Necessary becuase otherwise mypy errors with: There are no .py[i] files in directory 'contrib' Signed-off-by: Dan Callahan <danc@element.io>

Signed-off-by: Dan Callahan <danc@element.io>

DMRobertson

LGTM. We might want to ensure that the Python outside of the synapse package is linted too? But that's a separate change.

callahad · 2021-11-08T11:44:03Z

Would appreciate a second review before merging, since this does pretty significantly change our static type checking strategy across the repo.

reivilibre

I also think this is sensible and preferable — good to know that you can get the exclude functionality with a monster regex.

richvdh · 2021-11-08T12:17:07Z

mypy.ini

+  # Discover all .py[i] files in the repo
+  .,


won't this iterate into virtualenvs etc?

Mypy won't recurse into hidden directories, so as long as your venv is in .venv/ you're golden.

Are there any other common paths we should specifically exclude?

The README (and maybe docs) recommends putting the venv in an env directory with python3 -m venv ./env. I did this on day one.

Hm. Y'all should definitely switch to .venv, but I'll add an exclusion for env/ for now ;)

Happy to; I was just getting at that we ought to update the README! (and perhaps for sydent/sygnal too?)

.venv/ and env/ are the only things in gitignore, and first is implicitly excluded by having a dot. Now that env/ is also excluded, what else would you want?

basically anything that's not checked in - my whole point is that this shouldn't break when people have things in their working copy that you don't expect.

I guess maybe it would be ok to ignore things that are in the user's various gitignore configs, but that feels a bit magical.

In Apache voting terms, how strongly are your feelings on this issue?

Assume the alternative would be to only cover synapse/ and tests/ with mypy, allowing other files in the repo to go without type checking.

In Apache voting terms, how strongly are your feelings on this issue?

-0.9ish.

I like this way of resolving the dispute.

Assume the alternative would be to only cover synapse/ and tests/ with mypy, allowing other files in the repo to go without type checking.

Well, we could surely add other directories - scripts-dev, scripts, etc. As noted elsewhere, we already have to have multiple entries in the list.

mypy.ini

Signed-off-by: Dan Callahan <danc@element.io>

Prefixing the first entry with "|" technically adds an unnecessary exclusion for an empty path "^$", but retaining it since it will make diffs a little nicer. Signed-off-by: Dan Callahan <danc@element.io>

richvdh · 2021-11-08T14:05:18Z

mypy.ini

-  tests/util/test_itertools.py,
-  tests/util/test_stream_change_cache.py
+  # Discover all .py[i] files in the repo
+  .,


do we need to include .ci/scripts here?

richvdh · 2021-11-08T16:50:30Z

mypy.ini

+  # Discover all .py[i] files in the repo
+  .,
+
+  # Also check scripts that don't end in .py


synctl is missing from this list.

callahad · 2021-11-09T10:03:12Z

In light of Rich's objection to targeting the entire repo, let's go with the more minimal solution in #11282

callahad added 9 commits November 6, 2021 23:14

Invert mypy selections for synapse/

26fb2da

Signed-off-by: Dan Callahan <danc@element.io>

Invert mypy selections for tests/

53a451b

Signed-off-by: Dan Callahan <danc@element.io>

Invert mypy selections for scripts-dev/

0df2aba

Signed-off-by: Dan Callahan <danc@element.io>

Add scripts-dev/ files not ending in .py to mypy

0788539

Signed-off-by: Dan Callahan <danc@element.io>

Add scripts/ to mypy

755ea12

Signed-off-by: Dan Callahan <danc@element.io>

Add contrib/, docker/, and synmark/ to mypy

8984669

Signed-off-by: Dan Callahan <danc@element.io>

Make one script in contrib/ pass mypy

f5ebea8

Necessary becuase otherwise mypy errors with: There are no .py[i] files in directory 'contrib' Signed-off-by: Dan Callahan <danc@element.io>

Make setup.py pass mypy

4db6d96

Signed-off-by: Dan Callahan <danc@element.io>

All Python files must pass mypy unless excluded

e9baf37

Signed-off-by: Dan Callahan <danc@element.io>

callahad requested a review from a team as a code owner November 8, 2021 10:15

callahad added 2 commits November 8, 2021 10:16

Newsfragment

4aec13a

Signed-off-by: Dan Callahan <danc@element.io>

isort

f2178b2

Signed-off-by: Dan Callahan <danc@element.io>

DMRobertson approved these changes Nov 8, 2021

View reviewed changes

callahad requested a review from a team November 8, 2021 11:43

reivilibre approved these changes Nov 8, 2021

View reviewed changes

richvdh reviewed Nov 8, 2021

View reviewed changes

callahad added 3 commits November 8, 2021 12:35

Explain the verbose regex flag in a comment

6d22b83

Signed-off-by: Dan Callahan <danc@element.io>

Exclude all of contrib/ from mypy

8a973b7

Signed-off-by: Dan Callahan <danc@element.io>

Split up and comment the monster exclusion regex

515ca6b

Prefixing the first entry with "|" technically adds an unnecessary exclusion for an empty path "^$", but retaining it since it will make diffs a little nicer. Signed-off-by: Dan Callahan <danc@element.io>

richvdh reviewed Nov 8, 2021

View reviewed changes

mypy.ini

# Discover all .py[i] files in the repo

.,

# Also check scripts that don't end in .py

Copy link

Member

richvdh Nov 8, 2021

Choose a reason for hiding this comment

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

synctl is missing from this list.

callahad mentioned this pull request Nov 9, 2021

Require mypy for synapse/ & tests/ unless excluded #11282

Merged

4 tasks

callahad closed this Nov 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require all Python to pass mypy unless specifically excluded #11272

Require all Python to pass mypy unless specifically excluded #11272

callahad commented Nov 8, 2021 •

edited

Loading

DMRobertson left a comment

callahad commented Nov 8, 2021

reivilibre left a comment

richvdh Nov 8, 2021

callahad Nov 8, 2021

DMRobertson Nov 8, 2021

callahad Nov 8, 2021

DMRobertson Nov 8, 2021

callahad Nov 8, 2021

richvdh Nov 8, 2021

richvdh Nov 8, 2021

callahad Nov 8, 2021

richvdh Nov 8, 2021

richvdh Nov 8, 2021

callahad Nov 8, 2021

richvdh Nov 8, 2021

callahad commented Nov 9, 2021

Require all Python to pass mypy unless specifically excluded #11272

Require all Python to pass mypy unless specifically excluded #11272

Conversation

callahad commented Nov 8, 2021 • edited Loading

Pull Request Checklist

DMRobertson left a comment

Choose a reason for hiding this comment

callahad commented Nov 8, 2021

reivilibre 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

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

Choose a reason for hiding this comment

callahad commented Nov 9, 2021

callahad commented Nov 8, 2021 •

edited

Loading