Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Require all Python to pass mypy unless specifically excluded #11272

Closed
wants to merge 14 commits into from
Closed

Require all Python to pass mypy unless specifically excluded #11272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
26fb2da
Invert mypy selections for synapse/
callahad Nov 6, 2021
53a451b
Invert mypy selections for tests/
callahad Nov 6, 2021
0df2aba
Invert mypy selections for scripts-dev/
callahad Nov 6, 2021
0788539
Add scripts-dev/ files not ending in .py to mypy
callahad Nov 6, 2021
755ea12
Add scripts/ to mypy
callahad Nov 6, 2021
8984669
Add contrib/, docker/, and synmark/ to mypy
callahad Nov 7, 2021
f5ebea8
Make one script in contrib/ pass mypy
callahad Nov 7, 2021
4db6d96
Make setup.py pass mypy
callahad Nov 7, 2021
e9baf37
All Python files must pass mypy unless excluded
callahad Nov 7, 2021
4aec13a
Newsfragment
callahad Nov 8, 2021
f2178b2
isort
callahad Nov 8, 2021
6d22b83
Explain the verbose regex flag in a comment
callahad Nov 8, 2021
8a973b7
Exclude all of contrib/ from mypy
callahad Nov 8, 2021
515ca6b
Split up and comment the monster exclusion regex
callahad Nov 8, 2021
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
1 change: 1 addition & 0 deletions changelog.d/11272.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Require all new Python files in the repository to pass mypy.
4 changes: 2 additions & 2 deletions contrib/scripts/kick_users.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import json
import sys
import urllib
import urllib.parse
from argparse import ArgumentParser

import requests
Expand Down Expand Up @@ -56,7 +56,7 @@ def main(hs, room_id, access_token, user_id_prefix, why):
if len(doit) > 0 and doit.lower() == "y":
print("Kicking members...")
# encode them all
kick_list = [urllib.quote(uid) for uid in kick_list]
kick_list = [urllib.parse.quote(uid) for uid in kick_list]
for uid in kick_list:
kick_url = _mkurl(
"$HS/_matrix/client/api/v1/rooms/$ROOM/state/m.room.member/$UID?access_token=$TOKEN",
Expand Down
267 changes: 193 additions & 74 deletions mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,83 +9,202 @@ mypy_path = stubs
warn_unreachable = True
local_partial_types = True
no_implicit_optional = True
scripts_are_modules = True

# To find all folders that pass mypy you run:
#
# find synapse/* -type d -not -name __pycache__ -exec bash -c "mypy '{}' > /dev/null" \; -print
# To find files which are Python scripts, but which do not end in .py, run:
# rg --glob '!*.py' '^#!.*python' | cut -d ':' -f 1 | sort --unique

files =
scripts-dev/sign_json,
synapse/__init__.py,
synapse/api,
synapse/appservice,
synapse/config,
synapse/crypto,
synapse/event_auth.py,
synapse/events,
synapse/federation,
synapse/groups,
synapse/handlers,
synapse/http,
synapse/logging,
synapse/metrics,
synapse/module_api,
synapse/notifier.py,
synapse/push,
synapse/replication,
synapse/rest,
synapse/server.py,
synapse/server_notices,
synapse/spam_checker_api,
synapse/state,
synapse/storage/__init__.py,
synapse/storage/_base.py,
synapse/storage/background_updates.py,
synapse/storage/databases/main/appservice.py,
synapse/storage/databases/main/client_ips.py,
synapse/storage/databases/main/events.py,
synapse/storage/databases/main/keys.py,
synapse/storage/databases/main/pusher.py,
synapse/storage/databases/main/registration.py,
synapse/storage/databases/main/relations.py,
synapse/storage/databases/main/session.py,
synapse/storage/databases/main/stream.py,
synapse/storage/databases/main/ui_auth.py,
synapse/storage/databases/state,
synapse/storage/database.py,
synapse/storage/engines,
synapse/storage/keys.py,
synapse/storage/persist_events.py,
synapse/storage/prepare_database.py,
synapse/storage/purge_events.py,
synapse/storage/push_rule.py,
synapse/storage/relations.py,
synapse/storage/roommember.py,
synapse/storage/state.py,
synapse/storage/types.py,
synapse/storage/util,
synapse/streams,
synapse/types.py,
synapse/util,
synapse/visibility.py,
tests/replication,
tests/test_event_auth.py,
tests/test_utils,
tests/handlers/test_password_providers.py,
tests/handlers/test_room.py,
tests/handlers/test_room_summary.py,
tests/handlers/test_send_email.py,
tests/handlers/test_sync.py,
tests/handlers/test_user_directory.py,
tests/rest/client/test_login.py,
tests/rest/client/test_auth.py,
tests/rest/client/test_relations.py,
tests/rest/media/v1/test_filepath.py,
tests/rest/media/v1/test_oembed.py,
tests/storage/test_state.py,
tests/storage/test_user_directory.py,
tests/util/test_itertools.py,
tests/util/test_stream_change_cache.py
# Discover all .py[i] files in the repo
.,
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

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

won't this iterate into virtualenvs etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

do we need to include .ci/scripts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yep!


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

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.

scripts/export_signing_key,
scripts/generate_config,
scripts/generate_log_config,
scripts/hash_password,
scripts/register_new_matrix_user,
#scripts/synapse_port_db,
scripts/synapse_review_recent_signups,
#scripts/update_synapse_database,
scripts-dev/build_debian_packages,
scripts-dev/sign_json

# Note: Better exclusion syntax in mypy > 0.910
# See https://github.com/python/mypy/pull/11329
#
# For now, set the "(?x)" flag to use a "verbose" regex
# See https://docs.python.org/3/library/re.html#re.X
exclude = (?x)
^(
# Exclude common virtualenv locations
|env/.*

# Exclude contrib/, it is unsupported
|contrib/.*

# Exclude docker/ files that fail mypy
|docker/configure_workers_and_start.py

# Exclude scripts/ files that fail mypy
|scripts/move_remote_media_to_new_store.py

# Exclude scripts-dev/ files that fail mypy
|scripts-dev/check_signature.py
|scripts-dev/definitions.py
|scripts-dev/federation_client.py
|scripts-dev/hash_history.py
|scripts-dev/list_url_patterns.py
|scripts-dev/release.py
|scripts-dev/tail-synapse.py

# Exclude synapse/ files that fail mypy
|synapse/_scripts/
|synapse/app/
|synapse/storage/databases/__init__.py
|synapse/storage/databases/main/__init__.py
|synapse/storage/databases/main/account_data.py
|synapse/storage/databases/main/cache.py
|synapse/storage/databases/main/censor_events.py
|synapse/storage/databases/main/deviceinbox.py
|synapse/storage/databases/main/devices.py
|synapse/storage/databases/main/directory.py
|synapse/storage/databases/main/e2e_room_keys.py
|synapse/storage/databases/main/end_to_end_keys.py
|synapse/storage/databases/main/event_federation.py
|synapse/storage/databases/main/event_push_actions.py
|synapse/storage/databases/main/events_bg_updates.py
|synapse/storage/databases/main/events_forward_extremities.py
|synapse/storage/databases/main/events_worker.py
|synapse/storage/databases/main/filtering.py
|synapse/storage/databases/main/group_server.py
|synapse/storage/databases/main/lock.py
|synapse/storage/databases/main/media_repository.py
|synapse/storage/databases/main/metrics.py
|synapse/storage/databases/main/monthly_active_users.py
|synapse/storage/databases/main/openid.py
|synapse/storage/databases/main/presence.py
|synapse/storage/databases/main/profile.py
|synapse/storage/databases/main/purge_events.py
|synapse/storage/databases/main/push_rule.py
|synapse/storage/databases/main/receipts.py
|synapse/storage/databases/main/rejections.py
|synapse/storage/databases/main/room.py
|synapse/storage/databases/main/room_batch.py
|synapse/storage/databases/main/roommember.py
|synapse/storage/databases/main/search.py
|synapse/storage/databases/main/signatures.py
|synapse/storage/databases/main/state.py
|synapse/storage/databases/main/state_deltas.py
|synapse/storage/databases/main/stats.py
|synapse/storage/databases/main/tags.py
|synapse/storage/databases/main/transactions.py
|synapse/storage/databases/main/user_directory.py
|synapse/storage/databases/main/user_erasure_store.py
|synapse/storage/schema/

# Exclude synmark/ files that fail mypy
|synmark/__init__.py
|synmark/__main__.py
|synmark/suites/logging.py
|synmark/suites/lrucache.py
|synmark/suites/lrucache_evict.py

# Exclude tests/ files that fail mypy
|tests/api/test_auth.py
|tests/api/test_ratelimiting.py
|tests/app/test_openid_listener.py
|tests/appservice/test_scheduler.py
|tests/config/test_cache.py
|tests/config/test_tls.py
|tests/crypto/test_keyring.py
|tests/events/test_presence_router.py
|tests/events/test_utils.py
|tests/federation/test_federation_catch_up.py
|tests/federation/test_federation_sender.py
|tests/federation/test_federation_server.py
|tests/federation/transport/test_knocking.py
|tests/federation/transport/test_server.py
|tests/handlers/test_cas.py
|tests/handlers/test_directory.py
|tests/handlers/test_e2e_keys.py
|tests/handlers/test_federation.py
|tests/handlers/test_oidc.py
|tests/handlers/test_presence.py
|tests/handlers/test_profile.py
|tests/handlers/test_saml.py
|tests/handlers/test_typing.py
|tests/http/federation/test_matrix_federation_agent.py
|tests/http/federation/test_srv_resolver.py
|tests/http/test_fedclient.py
|tests/http/test_proxyagent.py
|tests/http/test_servlet.py
|tests/http/test_site.py
|tests/logging/__init__.py
|tests/logging/test_terse_json.py
|tests/module_api/test_api.py
|tests/push/test_email.py
|tests/push/test_http.py
|tests/push/test_presentable_names.py
|tests/push/test_push_rule_evaluator.py
|tests/rest/admin/test_admin.py
|tests/rest/admin/test_device.py
|tests/rest/admin/test_media.py
|tests/rest/admin/test_server_notice.py
|tests/rest/admin/test_user.py
|tests/rest/admin/test_username_available.py
|tests/rest/client/test_account.py
|tests/rest/client/test_events.py
|tests/rest/client/test_filter.py
|tests/rest/client/test_groups.py
|tests/rest/client/test_register.py
|tests/rest/client/test_report_event.py
|tests/rest/client/test_rooms.py
|tests/rest/client/test_third_party_rules.py
|tests/rest/client/test_transactions.py
|tests/rest/client/test_typing.py
|tests/rest/client/utils.py
|tests/rest/key/v2/test_remote_key_resource.py
|tests/rest/media/v1/test_base.py
|tests/rest/media/v1/test_media_storage.py
|tests/rest/media/v1/test_url_preview.py
|tests/scripts/test_new_matrix_user.py
|tests/server.py
|tests/server_notices/test_resource_limits_server_notices.py
|tests/state/test_v2.py
|tests/storage/test_account_data.py
|tests/storage/test_appservice.py
|tests/storage/test_background_update.py
|tests/storage/test_base.py
|tests/storage/test_client_ips.py
|tests/storage/test_database.py
|tests/storage/test_event_federation.py
|tests/storage/test_id_generators.py
|tests/storage/test_roommember.py
|tests/test_metrics.py
|tests/test_phone_home.py
|tests/test_server.py
|tests/test_state.py
|tests/test_terms_auth.py
|tests/test_visibility.py
|tests/unittest.py
|tests/util/caches/test_cached_call.py
|tests/util/caches/test_deferred_cache.py
|tests/util/caches/test_descriptors.py
|tests/util/caches/test_response_cache.py
|tests/util/caches/test_ttlcache.py
|tests/util/test_async_helpers.py
|tests/util/test_batching_queue.py
|tests/util/test_dict_cache.py
|tests/util/test_expiring_cache.py
|tests/util/test_file_consumer.py
|tests/util/test_linearizer.py
|tests/util/test_logcontext.py
|tests/util/test_lrucache.py
|tests/util/test_rwlock.py
|tests/util/test_wheel_timer.py
|tests/utils.py
)$

[mypy-synapse.api.*]
disallow_untyped_defs = True
Expand Down
6 changes: 4 additions & 2 deletions scripts-dev/build_debian_packages
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import subprocess
import sys
import threading
from concurrent.futures import ThreadPoolExecutor
from typing import Optional, Sequence
from typing import Optional, Sequence, Set, TextIO

DISTS = (
"debian:buster", # oldstable: EOL 2022-08
Expand Down Expand Up @@ -46,7 +46,7 @@ class Builder(object):
):
self.redirect_stdout = redirect_stdout
self._docker_build_args = tuple(docker_build_args or ())
self.active_containers = set()
self.active_containers: Set[str] = set()
self._lock = threading.Lock()
self._failed = False

Expand Down Expand Up @@ -76,6 +76,8 @@ class Builder(object):
debsdir = os.path.join(projdir, "../debs")
os.makedirs(debsdir, exist_ok=True)

stdout: Optional[TextIO]

if self.redirect_stdout:
logfile = os.path.join(debsdir, "%s.buildlog" % (tag,))
print("building %s: directing output to %s" % (dist, logfile))
Expand Down
1 change: 0 additions & 1 deletion scripts/export_signing_key
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ if __name__ == "__main__":
else format_plain
)

keys = []
for file in args.key_file:
try:
res = read_signing_keys(file)
Expand Down
5 changes: 2 additions & 3 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
# limitations under the License.
import glob
import os
from typing import Any, Dict

from setuptools import Command, find_packages, setup

Expand Down Expand Up @@ -49,8 +50,6 @@
# [1]: http://tox.readthedocs.io/en/2.5.0/example/basic.html#integration-with-setup-py-test-command
# [2]: https://pypi.python.org/pypi/setuptools_trial
class TestCommand(Command):
user_options = []

def initialize_options(self):
pass

Expand All @@ -75,7 +74,7 @@ def read_file(path_segments):

def exec_file(path_segments):
"""Execute a single python file to get the variables defined in it"""
result = {}
result: Dict[str, Any] = {}
code = read_file(path_segments)
exec(code, result)
return result
Expand Down