-
Notifications
You must be signed in to change notification settings - Fork 570
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
base: master
Are you sure you want to change the base?
Conversation
|
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. |
|
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. |
|
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 |
|
if only |
|
It's mainly those two. There's a few instances of other new types like |
You can do the same on Python 3.6 without |
|
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. |
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. |
|
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. |
|
I'm happy to get this PR green for you! |
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_sdkuvx 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