-
Notifications
You must be signed in to change notification settings - Fork 147
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 webpushpushkin.py #271
Conversation
webpushpushkin.py
webpushpushkin.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mypy appears to be unhappy:
sygnal/webpushpushkin.py:150: error: List item 0 has incompatible type "Optional[Any]"; expected "str" [list-item]
sygnal/webpushpushkin.py:180: error: List item 0 has incompatible type "Optional[Any]"; expected "str" [list-item]
sygnal/webpushpushkin.py:229: error: Argument 3 to "_handle_response" of "WebpushPushkin" has incompatible type "Optional[Any]"; expected "str" [arg-type]
sygnal/webpushpushkin.py:232: error: List item 0 has incompatible type "Optional[Any]"; expected "str" [list-item]
sygnal/webpushpushkin.py:250: error: Item "None" of "Optional[Any]" has no attribute "get" [union-attr]
I think most of these would be fixed by marking Device.pushkin: str
.
The last one seems to want an annotation for Device.data
.
(Perhaps you have these locally in another commit?)
sygnal/webpushpushkin.py
Outdated
import py_vapid | ||
from prometheus_client import Gauge, Histogram | ||
from py_vapid import Vapid, VapidException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it would be nicer to import py_vapid.CaseSensitiveDict
on line 26 of the RHS here, along with the other imports from that module.
So I mentioned this at the top of the PR that I think these errors will be resolved once the changes to |
Arg, sorry. You did indeed; I just dived straight into the diff. I think the changes you refer to are in #264 ? FWIW, I've found this workflow to be okayish for managing this kind of dependency: I've handled this in the past by starting a second branch based on the first change's branch. Then I target the PR at I typically end up
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Hopefully this is a little easier to parse. Just one file. There are a few mypy errors but I believe they will be resolved once the type hints for
notifications.py
are accepted and merged.