Additional type hinting

[sydduckworth]

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.
• Changed Compressor from an abstract class to a runtime-checkable protocol.
  • This changes the behavior of Compressor: previously a plugin only needed to implement one of compress or decompress but now plugins are required to implement both.
  • Plugins that only implement one of the required methods will continue to work but will generate a deprecation warning.
• 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]

Merging this PR will not alter performance

✅ 122 untouched benchmarks

---

_{Comparing sydduckworth:extension-type-checking (cf7be9d) with main (0aad9bd)}

[braingram]

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.

[sydduckworth]

Okay, I have reworked the way that the compression interface behaves:

• Compressor has been converted from an abstract class to a protocol. It now requires both compress and decompress.
• The new protocols are now private (_CompressionPlugin, _Compress, _Decompress)
• Internally when a compression plugin is registered ASDF will now check if it implements Compressor. If not it will check if it implements _Compress or _Decompress, in which case it will emit a deprecation warning. If it also doesn't implement those it will raise an exception as before.
• In the next major release we can fully remove support for the old interface at which point the private protocols can be folded into Compressor.
• As with the other protocols this means that Compressor no longer supports issubclass.

One thing that bugs me with this change is that the compress/decompress methods of Compressor are not abstract methods. This creates a situation where if I write a plugin that explicitly inherits from Compressor and forget to add one of the methods, the type checker won't complain (because I'm inheriting the "implementation" from Compressor) and there will be no runtime warning. Conversely, if I don't explicitly inherit from Compressor it will generate a runtime warning for the missing method.
Changing the methods to be abstract would be a breaking change because if someone is inheriting from Compressor they may be relying on the parent implementation, and making it abstract would mean the plugin could no longer be instantiated.
We could make them abstract now since it probably won't actually impact anyone downstream, or we could wait until the next major release, or we could leave it.

[zacharyburnett]

One thing that bugs me with this change is that the compress/decompress methods of Compressor are not abstract methods. This creates a situation where if I write a plugin that explicitly inherits from Compressor and forget to add one of the methods, the type checker won't complain (because I'm inheriting the "implementation" from Compressor) and there will be no runtime warning. Conversely, if I don't explicitly inherit from Compressor it will generate a runtime warning for the missing method. Changing the methods to be abstract would be a breaking change because if someone is inheriting from Compressor they may be relying on the parent implementation, and making it abstract would mean the plugin could no longer be instantiated. We could make them abstract now since it probably won't actually impact anyone downstream, or we could wait until the next major release, or we could leave it.

IMO conceptually I think making it abstract and enforced is better, but I also don't know what existing implementations there are. I'd make it a major release just in case since it is a breaking change of something public

[zacharyburnett]

I think the protocol-vs-abstract-implementation issue is important, but in order to not block the rest of this PR I think it's fine to make it a protocol now and then maybe enforce it with abstract later (if that would indeed be a breaking change we can maybe put it in an ASDF 6.0 milestone)

[sydduckworth]

@zacharyburnett agreed I think just changing it in the next major release makes the most sense. It's a breaking change but for it to actually be an issue someone would have to have implemented a custom compressor, inherited from Compressor, AND not defined compress or decompress. So I think the affected set of users should be pretty small if it exists.

[zacharyburnett]

would regression tests against jwst and romancal pick up any issues that the downstream CI wouldn't have?

[sydduckworth]

Seems unlikely that they would, but happy to run them just in case

• jwst
• romancal