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

ref(typing): Add additional stub packages for type checking #3122

Merged
merged 11 commits into from
Jun 25, 2024

Conversation

Daverball
Copy link
Contributor

@Daverball Daverball commented Jun 3, 2024

Adds types-webob, types-greenlet and types-gevent to linter requirements and fixes newly exposed typing issues.

Fixes #2226

@Daverball Daverball marked this pull request as ready for review June 4, 2024 15:19
Adds `types-webob`, `types-greenlet` and `types-gevent` to linter
requirements and fixes newly exposed typing issues.

Fixes getsentryGH-2226
@antonpirker antonpirker added the Component: DX Dealing with developer experience label Jun 6, 2024
Copy link
Contributor

@sentrivana sentrivana left a 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?

Comment on lines +56 to +58
from gevent.threadpool import ThreadPool as _ThreadPool

ThreadPool = _ThreadPool # type: Optional[Type[_ThreadPool]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

@Daverball Daverball Jun 25, 2024

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
Copy link
Contributor

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?

Copy link
Contributor Author

@Daverball Daverball Jun 25, 2024

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.

@sentrivana sentrivana enabled auto-merge (squash) June 25, 2024 11:30
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Jun 25, 2024
@sentrivana sentrivana added the Trigger: tests using secrets PR code is safe; run CI label Jun 25, 2024
@sentrivana sentrivana merged commit fca909f into getsentry:master Jun 25, 2024
118 of 121 checks passed
antonpirker pushed a commit that referenced this pull request Jun 25, 2024
Adds `types-webob`, `types-greenlet` and `types-gevent` to linter
requirements and fixes newly exposed typing issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: DX Dealing with developer experience Component: Typing Trigger: tests using secrets PR code is safe; run CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start using types-WebOb
3 participants