Skip to content
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

Add type hints for checking by mypy #355

Merged
merged 14 commits into from
Jun 9, 2021
Merged

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Jun 4, 2021

Here is the first round of adding support for mypy. A few things:

  • This PR resulted from my adding all the type annotations in the comments to various function and getting mypy to recognize them
  • I started trying to fix the errors that adding types produced but at some point realized it was too many changes at once/some of the changes were structural in ways I didn't feel confident about changing/etc. So I figured lets just get these baseline changes approved and merged and then go from there-i am sure there will be revisions
  • a lot of these changes were re-written as I learned more (using optional[whatever] instead of union[none, whatever], etc) so the final changes are in last committed version of each file
  • there are some files that already had annotations that I did not add, i did not change those
  • there is a directory http/servlets that I did not do, and i did not do sydent.py because it seemed like this PR was getting huge so i tried to stop somewhere that i could be clear on where to start again

signed off by H-Shay: shaysquared@gmail.com

@callahad callahad self-assigned this Jun 4, 2021
Copy link
Contributor

@callahad callahad left a comment

Choose a reason for hiding this comment

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

This is great! I have a few questions / suggested tweaks, and it looks like the black lint isn't passing (mainly Black wanting double quotes in places).

The unpaddedbase64 requirement in setup.py can be updated to >=1.1.0 to pull in a version which supplies type hints, avoiding the need for some type: ignore lines (best done in a followup pull request to ease review)

Generally, type: ignore lines are probably better replaced with a mypy.ini config file and set like:

[mypy-signedjson]
ignore_missing_imports = True

[mypy-unpaddedbase64]
ignore_missing_imports = True

Which has the same effect, but without all the individual annotations

sydent/db/accounts.py Outdated Show resolved Hide resolved
sydent/db/accounts.py Outdated Show resolved Hide resolved
sydent/db/hashing_metadata.py Outdated Show resolved Hide resolved
sydent/validators/msisdnvalidator.py Outdated Show resolved Hide resolved
sydent/http/auth.py Outdated Show resolved Hide resolved
sydent/http/httpclient.py Outdated Show resolved Hide resolved
sydent/http/matrixfederationagent.py Outdated Show resolved Hide resolved
sydent/http/matrixfederationagent.py Outdated Show resolved Hide resolved
sydent/http/matrixfederationagent.py Outdated Show resolved Hide resolved
sydent/http/matrixfederationagent.py Outdated Show resolved Hide resolved
@H-Shay
Copy link
Contributor Author

H-Shay commented Jun 8, 2021

This last commit should take care of the requested changes on this branch that aren't going to be moved to a separate PR. Thanks again for reviewing!

H-Shay added a commit to H-Shay/sydent that referenced this pull request Jun 8, 2021
@callahad
Copy link
Contributor

callahad commented Jun 8, 2021

Looks like flake8 in CI isn't totally happy yet, otherwise this is good to go once these are fixed:

sydent/http/httpsclient.py:21:1: F401 'typing.Generator' imported but unused
sydent/http/httpsclient.py:27:1: F401 'twisted.web.iweb.IResponse' imported but unused
sydent/http/srvresolver.py:24:1: F401 'twisted.internet.defer.Deferred' imported but unused
sydent/db/threepid_associations.py:19:1: F401 'typing.Sequence' imported but unused
sydent/db/hashing_metadata.py:20:1: F401 'typing.Union' imported but unused
sydent/util/ttlcache.py:18:1: F401 'typing.Dict' imported but unused
sydent/sms/openmarket.py:20:1: F401 'typing.Any' imported but unused
sydent/threepid/bind.py:22:1: F401 'typing.Generator' imported but unused
sydent/threepid/bind.py:22:1: F401 'typing.Optional' imported but unused
sydent/replication/peer.py:22:1: F401 'typing.Generator' imported but unused

Copy link
Contributor

@callahad callahad left a comment

Choose a reason for hiding this comment

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

Woohoo! This is an awesome step toward more confidence in refactoring Sydent. Thank you!

@callahad callahad merged commit 733272a into matrix-org:main Jun 9, 2021
H-Shay added a commit to H-Shay/sydent that referenced this pull request Jun 15, 2021
H-Shay added a commit to H-Shay/sydent that referenced this pull request Jun 15, 2021
Signed-off-by: H.Shay <shaysquared@gmail.com>
H-Shay added a commit to H-Shay/sydent that referenced this pull request Jun 15, 2021
… change was moved

to another PR.

This reverts commit e618512.
callahad pushed a commit that referenced this pull request Jun 15, 2021
per this: #355 (comment)

Signed-off-by: H.Shay <shaysquared@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants