Added type hints to _asdf.py and config.py#2031
Merged
Merged
Conversation
Member
|
this looks pretty comprehensive to me, I didn't know about PEP647 and type narrowing until just now 😅 |
zacharyburnett
approved these changes
Apr 30, 2026
Member
zacharyburnett
left a comment
There was a problem hiding this comment.
LGTM especially as there are no functional differences
braingram
approved these changes
Apr 30, 2026
Contributor
braingram
left a comment
There was a problem hiding this comment.
Thanks for putting this together and the detailed write-up. I left one comment about NOT_SET but it's not actionable at the moment. I didn't scrutinize all the hints and don't think that's necessary at this point (and with zach's review, thanks!).
|
|
||
| #: Special value indicating that a parameter is not set. | ||
| #: Distinct from None, which may for example be a value of interest in a search. | ||
| NOT_SET: Final = _NOT_SET_TYPE.NOT_SET |
Contributor
There was a problem hiding this comment.
We can yearn for the day we have 3.15 as a min: https://peps.python.org/pep-0661/
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
_asdf.pyandconfig.py. Added type hints to other modules as needed to support those two files.NotSetdescribed below)Nonebefore writing to itget_fileadded anisinstancecheck to the special case where the input is stdin/stdout/stderr so that type narrowing works correctlyget_fileupdatedRandomAccessFileand subclasses so that the reference to their original file handle is passed in the constructor instead of being set separatelyjsonschemawhich only existed to support Python versions < 3.9NotSetconstant in a way that plays nicely with the Python type systemdataclasses.MISSINGNotSetconstant has been renamed toNOT_SETbecause apparently a global being in all-caps makes type-checkers/linters assume it's immutableNotSetas a type alias for the typing needed to use theNOT_SETconstant.myparam: str | NotSet = NOT_SET. Usage in function bodies is the same and now doingvalue is not NOT_SETcorrectly applies type narrowing in the type-checker.typing.pymodule which contains new type aliases and protocols# pyrefly: ignorePotential Changes/Future Work
TreeKeyI added a
TreeKeytype alias inasdf.typingto be used for typing dictionaries that can be converted into ASDF trees.The original intention was that it would be
str | int | boolor similar, but it turns out thatThe end result is that you wouldn't be able to assign a
dict[str, Any]to adict[TreeKey, Any]field without manually widening thestrtype first.So for now I've just set
TreeKey = Any, with the idea that if we find a way in the future to narrow the type without breaking everything we can updateTreeKeyand not have to update every individual place its used.CompressionI added a type alias
ArrayStoragewhich aliases to the different string literal values that are valid forarray_storage. This has the nice effect that the Python LSP will auto-suggest the valid options when you're typing the value.I added another type alias
Compressionwhich was intended to do the same thing for the supported options forarray_compression.The complication is that array compression can be one of the predefined strings or it can also be any arbitrary string supported by an extension.
So I set
Compression = Literal["lz4", ...] | str | None, which works with the type-checker, but it looks like addingstrto the union means the LSP won't auto-suggest the valid options.I think the
Literal | strunion is still worth keeping rather than juststrsince it provides more information to the user and maybe in the future we can find a way to get it to work with the LSP.Documentation
asdf.typingto the User API section, but it might make more sense in the developer API section. Then again, with the addition of type hints we might want to rethink how we organize the API docs in the future anyway.Sorry about the size of this PR 😅