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 to sydent/http/servlets #361

Merged
merged 15 commits into from
Jun 21, 2021

Conversation

H-Shay
Copy link
Contributor

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

This PR request adds type hints to sydent/http/servlets. There were very few docstrings/comments so I did my best to figure out what the types were and add them.

@callahad callahad requested a review from a team June 16, 2021 21:01
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

I left one comment which is applicable to the entire PR, besides that I think this seems pretty good though!

sydent/http/servlets/__init__.py Outdated Show resolved Hide resolved
sydent/http/servlets/__init__.py Outdated Show resolved Hide resolved
@clokep clokep self-requested a review June 18, 2021 16:26
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

I think the main key here is that we should attempt to be a bit more consistent, some of the render_FOO functions return dict, sometimes Dict, sometimes Dict[str, Any] (or even more specific types).

It might help to define a JsonDict = Dict[str, Any] and then import that and return it as we do in Synapse.

sydent/http/servlets/blindlysignstuffservlet.py Outdated Show resolved Hide resolved
sydent/http/servlets/emailservlet.py Outdated Show resolved Hide resolved
sydent/http/servlets/threepidunbindservlet.py Outdated Show resolved Hide resolved
@H-Shay H-Shay requested a review from clokep June 18, 2021 20:46
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Almost there! Just a few minor changes!

sydent/http/servlets/__init__.py Outdated Show resolved Hide resolved
sydent/http/servlets/emailservlet.py Outdated Show resolved Hide resolved
sydent/http/servlets/msisdnservlet.py Outdated Show resolved Hide resolved
sydent/http/servlets/pubkeyservlets.py Outdated Show resolved Hide resolved
sydent/types.py Show resolved Hide resolved
@clokep
Copy link
Member

clokep commented Jun 21, 2021

Have we been removing the :type: fields in the docstrings as we do this or were we planning to do this afterward?

@H-Shay
Copy link
Contributor Author

H-Shay commented Jun 21, 2021

I haven't been removing the :type: fields in the docstrings but I sure can! I'll open another PR to do that though just so I'm not mixing in too many changes in this PR.

@H-Shay H-Shay requested a review from clokep June 21, 2021 15:43
@clokep
Copy link
Member

clokep commented Jun 21, 2021

I haven't been removing the :type: fields in the docstrings but I sure can! I'll open another PR to do that though just so I'm not mixing in too many changes in this PR.

That sounds like a good plan. (The reasoning to remove them is that there's no reason to encode the same info in two places, especially if one is enforced by a tool -- like mypy -- it tends to get out of date quickly.)

@clokep clokep merged commit 6626623 into matrix-org:main Jun 21, 2021
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.

2 participants