- Notifications
You must be signed in to change notification settings - Fork 578
Convert sentry_sdk type annotations into the modern format#5206
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
Convert sentry_sdk type annotations into the modern format #5206
Uh oh!
There was an error while loading. Please reload this page.
Conversation
zsol commented Dec 9, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
alexander-alderman-webb commented Dec 11, 2025
Hi @zsol, Thank you for the PR. Upgrading the type annotation style is a breaking change because we still support Python 3.6. We will do this work on the major branch, and can keep you updated. |
charliermarsh commented Dec 11, 2025
My understanding is that PEP 526 is compatible with Python 3.6 (but not Python 3.5), so IIUC this shouldn't break any Python 3.6 users. |
alexander-alderman-webb commented Dec 12, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
You're correct that using PEP 526 is possible on Python 3.6. The reason we didn't do the type transition yet is that we use some newer types, such as typing.Literal and typing.TypedDict. We currently type check on newer Python versions, while supporting 3.6 at runtime for users. This is possible because old-style type annotations together with You can achieve an analogous result on Python 3.7 with new-style type annotations, but I believe you require |
sl0thentr0py commented Dec 12, 2025
if only |
alexander-alderman-webb commented Dec 12, 2025
It's mainly those two. There's a few instances of other new types like |
AlexWaygood commented Dec 12, 2025
You can do the same on Python 3.6 without |
alexander-alderman-webb commented Dec 12, 2025
Great to know! CI seems to fail because there are some places in which the annotations are not in quotes in this PR. What is the motivation for the PR? We need to transition regardless, but for us to prioritize bringing this over the line it would be helpful to understand if there are limitations beyond comment-based types becoming less idiomatic. |
AlexWaygood commented Dec 12, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Ah, cool, that's a bug in the PR! Thanks for the heads-up (cc. @zsol)
Yes. The main motivation is that newer type checkers such as ty do not support the legacy syntax. (Full disclosure: I'm a ty developer and Zsolt's a colleague, though he doesn't work on ty.) It would be quite a bit of work for us to add support for the legacy syntax to ty, so we're reluctant to do so given that it's only required for compatibility with Python <=3.5. Meanwhile, we're big fans of Sentry, and have other code elsewhere that uses Sentry. We'd love to type-check that code with ty, but doing so currently produces poor results, because ty can't understand any of Sentry's legacy type hints. Note that there have also been discussions about deprecating the legacy syntax from the type system altogether, albeit those discussions have been dormant for a while now:
Other tools such as pyflakes/flake8 dropped support for type comments a while back, and Ruff has never supported them. |
alexander-alderman-webb commented Dec 12, 2025
Thank you for the detailed explanation, and thank you for using Sentry! We didn't transition yet because of internal misunderstanding that we need The steps already in the PR descriptions are great since we'll verify the manual fixes. If you have a systematic way of putting all annotations in quotes keep us updated. Otherwise, I'll pick up from here next week. |
zsol commented Dec 12, 2025
I'm happy to get this PR green for you! |
af15feb to d1eb610Comparezsol commented Dec 15, 2025
I believe I've resolved all the unquoted type annotations and pulled in changes from master. Let's see what CI says |
alexander-alderman-webb commented Dec 15, 2025
Great, thanks for making CI green! I'll run some diffs and then merge if everything looks good. |
codecovbot commented Dec 15, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## master #5206 +/- ## ========================================== - Coverage 84.25% 84.24% -0.02% ========================================== Files 181 182 +1 Lines 18463 18550 +87 Branches 3288 3299 +11 ========================================== + Hits 15556 15627 +71 - Misses 1894 1911 +17 + Partials 1013 1012 -1
|
973dda7 into getsentry:masterUh oh!
There was an error while loading. Please reload this page.
zsol commented Dec 15, 2025
Thanks for the quick turnaround @alexander-alderman-webb! |
Description
This PR converts all Python 2-style type annotations (in comments) into Python 3-style annotations (after
:tokens, and inside the AST).There's a large amount of changes in here, apologies for that. I generated these changes using:
uv run --with libcst python -m libcst.tool codemod convert_type_comments.ConvertTypeComments --no-format -j1 sentry_sdkuvx com2ann sentry_sdkast-grep scan --rule $PATH_TO_THE_YML sentry_sdk --interactive)uvx ruff format sentry_sdkManual changes
foo = None # type: Foo # noqaintofoo: "Foo # noqa" = None, which were trivial to fixfoo = None # type: Foointofoo: Foo = None, mypy starts raising an error on the second form - I suppressed these usingtype: ignore[assignment]Test plan
mypyshould continue being green, as evidenced by runninguvx tox -e linters