Repository navigation
Avoid stringified type annotations in units - #18329
Conversation
eerovaher
added
units
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.
|
nstarman
left a comment
nstarman
left a comment
There was a problem hiding this comment.
I rearranged stuff as suggested, and the diff looks good.
PEP 649 is going to clean up all the stringified annotations in py 3.13+ but overall there are actually now fewer such annotations because of the removal of from __future__ import annotations.
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Yes, looks good. It slightly breaks having required functions defined before their use, but on balance for UnitBase it is also nice that it gets defined earlier.
For PhysicalType, though, perhaps still after the definitions? (see in-line)
Also, one question just to be sure: currently, inside the UnitBase definition, for typing still "UnitBase" is used. Is it really necessary to even stringify the name of the class being defined? It seems in this case it is not a forward reference, so should be OK. (Though perhaps the class needs to actually have been completely defined.) Anyway, just in case -- you probably checked this already!
| __array__: Final = None | ||
|
|
||
|
|
||
| _units_and_physical_types: Final[list[tuple[core.UnitBase, str | set[str]]]] = [ |
There was a problem hiding this comment.
I'd have a slight preference for keeping this list on top, since it is the thing one needs to edit if a new physical type is added. Since it doesn't include PhysicalType anywhere, presumably that should not affect anything.
There was a problem hiding this comment.
It would be best if the physical types definitions and machinery were in separate modules and I even have a pull request to make that change, but it's stuck because I can't figure out how to make the API docs build without warnings. I don't see much value in moving these definitions within this module unless we entirely give up the idea of having separate modules.
There was a problem hiding this comment.
It would be best if the physical types definitions and machinery were in separate modules
Agreed.
I even have a pull request to make that change
🎉
but it's stuck because I can't figure out how to make the API docs build without warnings
Sounds familiar.
I don't see much value in moving these definitions within this module unless we entirely give up the idea of having separate modules.
I think it's about keeping aspects of the current pre-PR layout.
It is necessary and it is not specific to annotations: Python 3.12.3 (main, Jun 18 2025, 17:59:45) [GCC 13.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> class A:
... a = A
...
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<stdin>", line 2, in A
NameError: name 'A' is not defined |
|
@eerovaher if you rebase, let's get this in. |
Sphinx is known to have problems resolving stringified annotations, so it's best to avoid them whenever possible. In `units` it is not possible to avoid stringified annotations entirely because of circular imports, but it was nonetheless possible to remove `if TYPE_CHECKING:` blocks from all but two modules. Moving the definitions of `UnitBase` and `PhysicalType` was necessary for minimizing the number of forward references.
e73f3b3 to
983beb3
Compare
|
OK, thanks, let's get it in! Note, though, that to me this change seems to invasive for backporting (even if functionally nothing changes), so I removed that. If we decide that was wrong, we could still backport manually. |
|
Downstream has already reported documentation build failures caused by stringified annotations (#18221), so not backporting is irresponsible. |
|
OK, fair enough. Fine to backport with a |
|
@meeseeksdev backport to v7.1.x |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
|
Re: #18329 (comment)
Thank you! |
The backport is required to prevent/solve problems analogous to those reported in astropy#18221.
Backport PR #18329 on branch v7.1.x (Avoid stringified type annotations in units)
Description
Sphinx is known to have problems resolving stringified annotations, so it's best to avoid them whenever possible. In
unitsit is not possible to avoid stringified annotations entirely because of circular imports, but it was nonetheless possible to removeif TYPE_CHECKING:blocks from all but two modules. Moving the definitions ofUnitBaseandPhysicalTypewas necessary for minimizing the number of forward references, but that makes the diff look huge. I recommend the reviewers first moveUnitBaseandPhysicalTypedefinitions in their local repositories and then compare the resulting working tree with the branch here. That way the amount of code changes should be quite manageable.