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

Refactor HomeserverConfig so it can be typechecked #6137

Merged
merged 34 commits into from
Oct 10, 2019

Conversation

hawkowl
Copy link
Contributor

@hawkowl hawkowl commented Sep 30, 2019

This means that static typing can now functionally be used, if we define type stubs/mypy can infer things

@hawkowl hawkowl requested a review from a team October 3, 2019 13:54
@hawkowl hawkowl marked this pull request as ready for review October 3, 2019 13:54
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

looks good, but a few questions

@@ -52,3 +48,15 @@ ignore_missing_imports = True

[mypy-signedjson.*]
ignore_missing_imports = True

[mypy-prometheus_client.*]
Copy link
Member

Choose a reason for hiding this comment

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

why are we adding this sort of thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we don't have type stubs for these modules, so therefore we don't want to typecheck them. We don't want to ignore missing imports completely, just on dependencies without type hints.

Copy link
Member

Choose a reason for hiding this comment

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

ok, but it just doesn't seem related to the subject of the PR and the text of the changelog.

I guess it's necesary because of https://github.com/matrix-org/synapse/pull/6137/files#diff-b91f3d5bd63fcd17221b267e851608e8R169?

synapse/config/_base.py Outdated Show resolved Hide resolved
synapse/config/_base.py Outdated Show resolved Hide resolved
synapse/config/_base.py Outdated Show resolved Hide resolved
synapse/config/_base.py Show resolved Hide resolved
synapse/config/_base.py Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
@hawkowl hawkowl requested review from richvdh and a team and removed request for richvdh October 8, 2019 13:43
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm.

@hawkowl hawkowl merged commit f743108 into develop Oct 10, 2019
@hawkowl hawkowl deleted the hawkowl/config-cleanup branch October 10, 2019 08:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants