-
Notifications
You must be signed in to change notification settings - Fork 500
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
ref(typing): Add additional stub packages for type checking #3122
ref(typing): Add additional stub packages for type checking #3122
Conversation
100da78
to
e247d0c
Compare
Adds `types-webob`, `types-greenlet` and `types-gevent` to linter requirements and fixes newly exposed typing issues. Fixes getsentryGH-2226
e247d0c
to
0d3d273
Compare
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.
Thanks @Daverball, looks good to me overall. I left two comments, could you please take a look?
from gevent.threadpool import ThreadPool as _ThreadPool | ||
|
||
ThreadPool = _ThreadPool # type: Optional[Type[_ThreadPool]] |
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.
Why is this necessary?
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.
If we don't do this, then mypy will complain about redefinition of ThreadPool
to None
in the except
clause. So the type and the variable need a separate name, so I can use the type separately from the runtime variable.
@@ -54,6 +54,8 @@ | |||
Union, | |||
) | |||
|
|||
from gevent.hub import Hub |
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 gevent
is an optional dependency, won't this break type checking for folks that don't have it installed? Would a conditional import help in any way?
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's no conditional imports for typing unfortunately. So yes, if people leave follow_imports
at normal
for sentry_sdk
they will see an error if they don't have types-gevent
installed, but the same is true for any of the other typing only dependencies that are only listed in dev-requirements.txt
, so this would be nothing new.
Generally the expectation with type hints is that people are responsible for managing typing-
requirements and otherwise silence the errors by setting follow_imports
to silent
or skip
.
Adds `types-webob`, `types-greenlet` and `types-gevent` to linter requirements and fixes newly exposed typing issues.
…y#3122) Adds `types-webob`, `types-greenlet` and `types-gevent` to linter requirements and fixes newly exposed typing issues.
Adds
types-webob
,types-greenlet
andtypes-gevent
to linter requirements and fixes newly exposed typing issues.Fixes #2226