-
Notifications
You must be signed in to change notification settings - Fork 149
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 sygnal.py
#275
Conversation
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. I think the only two changes I'm particularly keen on are the Tracer and the #type: ignore[code]
one. The rest are very minor or comments for interest.
sygnal/sygnal.py
Outdated
self, | ||
config: Dict[str, Any], | ||
custom_reactor: SygnalReactor, | ||
tracer: Union[Tracer, jaeger_client.Tracer] = opentracing.tracer, |
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.
I think you should be able to just use Tracer
, because jaeger_client.Tracer inherits from opentracing.Tracer.
tests/testutils.py
Outdated
self.sygnal = Sygnal(config, reactor) # type: ignore | ||
self.reactor = reactor | ||
|
||
start_deferred = ensureDeferred(self.sygnal.make_pushkins_then_start()) | ||
|
||
while not start_deferred.called: | ||
# we need to advance until the pushkins have started up | ||
assert isinstance(self.sygnal.reactor, ExtendedMemoryReactorClock) | ||
self.sygnal.reactor.advance(1) |
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.
Since we already have the reactor, maybe use self.reactor.advance
etc. here and in the two cases below. I think that will let us avoid the assertions.
(Nothing wrong with as it is, just trying to keep things concise where possible)
tests/test_http.py
Outdated
assert isinstance(value, ApnsPushkin) | ||
# type safety: ignore is used here due to mypy not handling monkeypatching, | ||
# see https://github.com/python/mypy/issues/2427 | ||
value._send_notification = self.apns_pushkin_snotif # type: ignore |
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.
Where possible, prefer #type: ignore[code]
where code
is one of mypy's error codes. See https://mypy.readthedocs.io/en/stable/error_codes.html and the two pages following it for gory details.
(but easier to remove the comment and see what the error code you get in the terminal is.)
Thanks @H-Shay, looks great! |
As the title says.