Repository navigation
Conversation
`CoordinateTransform` and its subclasses now have type annotations.
The code inside the deleted `with suppress(TypeError):` block was guaranteed to raise a `TypeError` because it tried to check the length of a generator.
eerovaher
added
coordinates
no-changelog-entry-needed
typing
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
| with suppress(TypeError): | ||
| sig = signature(func) | ||
| kinds = [x.kind for x in sig.parameters.values()] | ||
| if ( | ||
| len(x for x in kinds if x == sig.POSITIONAL_ONLY) != 2 | ||
| and sig.VAR_POSITIONAL not in kinds | ||
| ): | ||
| raise ValueError("provided function does not accept two arguments") |
There was a problem hiding this comment.
Trying to check the length of a generator raises a TypeError, which means this entire block is a no-op. Replacing the generator expression with a list comprehension would solve that error, but then trying to access sig.POSITIONAL_ONLY and sig.VAR_POSITIONAL would raise an AttributeError.
|
I'm very busy teaching until December, and will not be able to review as many PRs as usual; I hope it will be possible to pass this one to someone else, ideally someone with more interest/experience in typing. |
|
At this point it's quite clear that this isn't going to get merged to v7.2. I do not want this merged right after the feature freeze. The annotations can be made much more informative with PEP 695 syntax, which we can start using when we drop support for Python 3.11. |
nstarman
left a comment
nstarman
left a comment
There was a problem hiding this comment.
Largely LGTM. One comment about imports in TYPE_CHECKING blocks, which we should also address later in other parts of the code.
| from astropy.coordinates.typing import Matrix3x3 | ||
|
|
||
| if TYPE_CHECKING: | ||
| from astropy.coordinates.baseframe import BaseCoordinateFrame |
There was a problem hiding this comment.
Based on eg https://github.com/xarray/xarray/issues/9581
We should probably be moving towards full path forward annotations, until Python 3.14 is our minimum version.
import astropy.coordinates
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
nstarman
added
keep-open
and removed
Close?
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately a year. I plan to close this in a month if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
Description
The first commit adds initial type annotations to
CoordinateTransformand its subclasses. In many cases I rewrote type information from docstrings as annotations. If there are mistakes in the docstrings then the annotations will contain the same mistakes. I did check some of the types at runtime by adding temporaryisinstance()checks to the code and running thecoordinatestests, but this might not catch mistakes if test coverage is incomplete. I also usedto ensure the new annotations are self-consistent, but that ignores unannotated code, so there is still the possibility that adding more annotations elsewhere in
coordinateswill reveal typing problems here.Mypy did reveal that a code block in
FunctionTransformis a no-op. The second commit here removes the block in question because otherwise Mypy would keep complaining about it.I am not annotating
TransformGraphyet because this pull request is large enough as it is and also because I don't want to create merge conflicts with #18695.