Skip to content

Additional type hinting - #2114

Open
sydduckworth wants to merge 10 commits into
asdf-format:mainfrom
sydduckworth:extension-type-checking
Open

Additional type hinting#2114
sydduckworth wants to merge 10 commits into
asdf-format:mainfrom
sydduckworth:extension-type-checking

Conversation

@sydduckworth

@sydduckworth sydduckworth commented Aug 10, 2026

Copy link
Copy Markdown
Member

Sorry about the size of this PR! Typing changes just kept cascading into each other 😭

Description

This PR adds type hints to a significant portion of the codebase. It also converts/replaces all of the abstract classes that use __subclasshook__ with protocols that work with type-checking.

Compared to my previous type hint PR this PR required notably fewer functional code changes to avoid typing errors, which is probably a good sign.

Interface/Protocol Changes

  • Changed Converter from an abstract class to a runtime-checkable protocol.
    • Technically this is a breaking change because although runtime-checkable protocols support isinstance they don't support issubclass if they have non-method members (which includes properties). I am assuming/hoping no external code is doing issubclass(Converter, ...). If this is a problem we can revert Converter and add a new protocol with a different name that Converter inherits from (the reason to not just do this to start with is it adds more complexity and a disconnect between runtime and type-checking).
    • Converter (and ConverterProxy) are now generic over both object type and node type, with node type defaulting to YamlNode. For example, a converter that converts MyClass into arbitrary YAML could inherit Converter[MyClass]. A converter that converts datetime into a string could inherit Converter[datetime, str]. Surprisingly, Pyrefly seems to be able to infer the generic types for protocols decently well, even when dealing with duck-typed implementations.
    • Added type hints and generics to all converter implementations.
  • Factored Compressor into multiple protocols for type-checking, while keeping the existing Compressor for backwards compatibility.
    • This is necessary because the subclass hook for Compressor requires subclasses to implement compress or decompress but does not require both, which isn't something that can really be expressed in a single Python protocol.
    • CompressorPlugin contains the core label property.
    • Compress contains the compress method.
    • Decompress contains the decompress method.
    • Internal code now uses the three protocols for type hints and type-checking. Compressor inherits from both Compress and Decompress so anything inheriting from Compressor will still work the same.
    • Deprecated Compressor in favor of the new protocols.
  • Updates to Extension
    • Moved ExtensionLike protocol from asdf.typing to asdf.extension. There hasn't been a release since it was added so this should be fine
    • Updated Extension and ExtensionProxy to inherit from ExtensionLike (ExtensionProxy should not have been subclassing Extension because it has methods with the same names but meaningfully different return types!)
    • Updated internal type hints to use ExtensionLike where any extension object works and ExtensionProxy where the full set of methods are required.

General Changes

  • Added type hints to asdf.extension module and submodules
  • Added type hints to converter modules
  • Added some type hints (but not complete coverage) to resource.py, versioning.py, _compression.py, and _node_info.py.
  • Updated tests to work with new type hints (mostly test_extension.py)
  • Updated documentation on extending compressors to reflect the new compression protocols.

AI Disclosure

No AI tools used

Tasks

  • run prek on your machine
  • run pytest on your machine
  • Does this PR add new features and / or change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update relevant docstrings and / or docs/ page
    • for any new features, add unit tests
news fragment change types...
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: bug fix
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

@codspeed-hq

codspeed-hq Bot commented Aug 12, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 122 untouched benchmarks


Comparing sydduckworth:extension-type-checking (dce9804) with main (5fc55f7)

Open in CodSpeed

@sydduckworth
sydduckworth marked this pull request as ready for review August 12, 2026 17:17
@sydduckworth
sydduckworth requested a review from a team as a code owner August 12, 2026 17:17
@braingram

Copy link
Copy Markdown
Contributor

As you noted there is a lot in this PR which will take some time to review. Some general comments based on the description:

  • Switching to protocols: The loss of issubclass support is more a theoretical issue as far as I can tell (I don't see any usage in our downstream even though we use it in tests here). It is a loss of functionality which is unfortunate. Since it has no functional impact and protocols seems to be the way the community is moving this seems reasonable. It would have been easier to review this as a separate PR.
  • Breaking Compressor into 2 protocols: Is there any Compressor that only implements compress or decompress and not the other? If not (or even if there is an example) perhaps we deprecate that option and require that Compressors implement both? Splitting it seems clunky since as far as I can tell all of the ones we implement here in asdf and asdf-compression support both and if we split them we'd need either 3 classes (1 base to define the labels, 2 for the compressor, decompressor), multiple-inheritance, or duplicating label definitions. It's simpler to have this all be in 1 class/protocol. If keeping the existing behavior (only require 1, until we can deprecate it) means we can't add type hints I'd say we don't type hint Compressor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants