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

Type hints for apnstruncate, apnspushkin, notifications, and gcmpushkin #264

Merged
merged 19 commits into from
Nov 16, 2021

Conversation

H-Shay
Copy link
Contributor

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

Here is my first try at adding some type hints to the below files in Sygnal, and making very minor modifications to the code to silence some mypy errors (like assert is not None).
A very small handful of errors remain (less than 4 I think?) across the files I changed but silencing them seemed like it might require more modification to the code, and I wanted to make sure that everything I had here was correct before I started tackling those.
I think that these are mostly correct but I find typing to be a little difficult so it is entirely possible I have gotten some things wrong.

@H-Shay H-Shay requested a review from a team as a code owner November 4, 2021 21:46
@reivilibre reivilibre self-assigned this Nov 5, 2021
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this!
I think you might be noticing that trying to bite a lot at once will pull you into everywhere else in the code because everything's linked together.
A couple of places where you've used cast I've tried to replace with actual checks — cast should be thought of as 'oi mypy, you're too stupid to see that I'm right so just trust me unconditionally' — in general, we try to avoid it.

sygnal/apnspushkin.py Outdated Show resolved Hide resolved
sygnal/apnspushkin.py Outdated Show resolved Hide resolved
sygnal/apnspushkin.py Outdated Show resolved Hide resolved
sygnal/apnspushkin.py Outdated Show resolved Hide resolved
sygnal/apnspushkin.py Outdated Show resolved Hide resolved
sygnal/gcmpushkin.py Outdated Show resolved Hide resolved
sygnal/notifications.py Outdated Show resolved Hide resolved
sygnal/notifications.py Outdated Show resolved Hide resolved
sygnal/notifications.py Outdated Show resolved Hide resolved
sygnal/notifications.py Outdated Show resolved Hide resolved
sygnal/apnspushkin.py Outdated Show resolved Hide resolved
sygnal/apnspushkin.py Outdated Show resolved Hide resolved
sygnal/apnspushkin.py Outdated Show resolved Hide resolved
sygnal/apnspushkin.py Outdated Show resolved Hide resolved
sygnal/gcmpushkin.py Outdated Show resolved Hide resolved
sygnal/gcmpushkin.py Outdated Show resolved Hide resolved
sygnal/apnspushkin.py Outdated Show resolved Hide resolved
sygnal/notifications.py Outdated Show resolved Hide resolved
sygnal/gcmpushkin.py Outdated Show resolved Hide resolved
@squahtx
Copy link
Contributor

squahtx commented Nov 5, 2021

For the tuple index out of range complaints, we might want to define
Choppable = Union[Tuple[Literal["alert", "alert.body"]], Tuple[Literal["alert.loc-args"], int]]
(ie. Choppable is either ("alert",), ("alert.body",) or ("alert.loc-args", int))
then mypy should be clever enough to figure things out.

sygnal/apnspushkin.py Outdated Show resolved Hide resolved
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

there are merge conflicts, otherwise very close to merge I think

sygnal/gcmpushkin.py Outdated Show resolved Hide resolved
sygnal/apnspushkin.py Show resolved Hide resolved
reivilibre
reivilibre previously approved these changes Nov 12, 2021
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

sygnal/apnspushkin.py Outdated Show resolved Hide resolved
@H-Shay H-Shay removed the request for review from reivilibre November 15, 2021 03:22
@H-Shay H-Shay marked this pull request as draft November 15, 2021 03:23
@H-Shay H-Shay marked this pull request as ready for review November 15, 2021 18:34
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

Thank you!

@H-Shay H-Shay merged commit c1a2969 into main Nov 16, 2021
@H-Shay H-Shay deleted the type_hints branch November 16, 2021 15:32
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.

4 participants