Convert sentry_sdk type annotations into the modern format
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:
- A LibCST codemod to do the bulk of the work:
uv run --with libcst python -m libcst.tool codemod convert_type_comments.ConvertTypeComments --no-format -j1 sentry_sdk - Some manual fixups here and there (see below)
- Run com2ann to fix the rest:
uvx com2ann sentry_sdk - Reformat:
uvx ruff format sentry_sdk
Manual changes
- The LibCST command made a bunch of silly mistakes like turning
foo = None # type: Foo # noqaintofoo: "Foo # noqa" = None, which were trivial to fix - When turning
foo = None # type: Foointofoo: Foo = None, mypy starts raising an error on the second form - I suppressed these usingtype: ignore[assignment]
Test plan
mypy should continue being green, as evidenced by running uvx tox -e linters
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 typing.TYPE_CHECKING prevent user runtimes from having to import types.
You can achieve an analogous result on Python 3.7 with new-style type annotations, but I believe you require __future__.annotations, which is unfortunately only available in Python 3.7. See https://peps.python.org/pep-0563/.
if only Literal and TypedDict are the blockers, I'm fine with removing those since I massively dislike TypedDict anyway
It's mainly those two. There's a few instances of other new types like typing.NotRequired and typing_extensions.Unpack.
You can achieve an analogous result on Python 3.7 with new-style type annotations, but I believe you require
__future__.annotations, which is unfortunately only available in Python 3.7. See peps.python.org/pep-0563.
You can do the same on Python 3.6 without __future__.annotations if you're happy to use stringized/quoted annotations. Which this PR does.
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. Are there new tools for which the comment-based annotations do not work?
Great to know! CI seems to fail because there are some places in which the annotations are not in quotes in this PR.
Ah, cool, that's a bug in the PR! Thanks for the heads-up (cc. @zsol)
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. Are there new tools for which the comment-based annotations do not work?
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:
- https://mail.python.org/archives/list/[email protected]/thread/66JDHQ2I3U3CPUIYA43W7SPEJLLPUETG/
- https://github.com/python/mypy/issues/12947
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 __future__.annotations. We are happy to support your efforts so that others can use ty with sentry-python.
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!
I believe I've resolved all the unquoted type annotations and pulled in changes from master. Let's see what CI says
Great, thanks for making CI green!
I'll run some diffs and then merge if everything looks good.
Codecov Report
:x: Patch coverage is 98.83659% with 22 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 84.24%. Comparing base (2ce4379) to head (a7313ae).
:warning: Report is 16 commits behind head on master.
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
| Files with missing lines | Coverage Δ | |
|---|---|---|
| sentry_sdk/_compat.py | 94.87% <100.00%> (ø) |
|
| sentry_sdk/_init_implementation.py | 95.83% <100.00%> (ø) |
|
| sentry_sdk/_log_batcher.py | 80.89% <100.00%> (ø) |
|
| sentry_sdk/_lru_cache.py | 100.00% <100.00%> (ø) |
|
| sentry_sdk/_metrics_batcher.py | 73.86% <100.00%> (ø) |
|
| sentry_sdk/_queue.py | 62.22% <100.00%> (ø) |
|
| sentry_sdk/_werkzeug.py | 48.14% <100.00%> (ø) |
|
| sentry_sdk/ai/monitoring.py | 85.33% <100.00%> (ø) |
|
| sentry_sdk/ai/utils.py | 94.68% <100.00%> (ø) |
|
| sentry_sdk/api.py | 86.57% <100.00%> (ø) |
|
| ... and 153 more |
Thanks for the quick turnaround @alexander-alderman-webb!