index converters by type when no converter is found #1654
Merged
Conversation
This file contains 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
braingram
force-pushed
the
class_path
branch
2 times, most recently
from
3e5cdd6 to
9cef8ef
Compare
braingram
force-pushed
the
class_path
branch
3 times, most recently
from
6d4f55a to
af27436
Compare
braingram
force-pushed
the
class_path
branch
from
af27436 to
04414d5
Compare
braingram
force-pushed
the
class_path
branch
3 times, most recently
from
2e1fd13 to
a44def4
Compare
5 tasks
zacharyburnett
approved these changes
May 8, 2024
This changes how converters with class paths/names as types are handled by asdf. Prior to this commit, when converting a class instance (foo) of class (Foo) the class path of the instance was inspected with ``get_class_name`` and the ``_converters_by_type`` index was searched using the class name. This index contained both classes and class paths (as converters could use both/either) with classes being preferred. This creates issues when a module has a different class path from the typical 'public' path for the class as the module might move the private implementation without changing the 'public' path with the expectation that this will not break downstream code. For converters that use class paths these types of moves will break the converter. This commit changes the handling of class paths used for converters so that the 'public' path can be used. If a converter is not found for a class in ``_converters_by_type`` (which now only contains classes as keys) then ``_converters_by_class_path`` is indexed by checking what classes referred to by the paths are already imported (to avoid importing every class supported by every extension). If the class is imported it is added to ``_converters_by_type`` and removed from ``_converters_by_class_path``.
this fixes an issue with extension order and class path. If extension 1 is found first, and registers a converter using a class path (with a module that hasn't been imported). Then, extension 2 registers the same class but uses the class (not the path). Prior to this commit (in this branch) asdf would use extension 2 for the type (because the class path registered with extension 1 was never indexed). With this commit all converter class paths/types are recorded and then indexed (in the order they're registered) fixing this issue.
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.
This changes how converters with class paths/names as types are handled by asdf.
Prior to PR, when converting a class instance (foo) of class (Foo) the class path of the instance was inspected with
get_class_nameand the_converters_by_typeindex was searched using the class name. This index contained both classes and class paths (as converters could use both/either) with classes being preferred. This creates issues when a module has a different class path from the typical 'public' path for the class as the module might move the private implementation without changing the 'public' path with the expectation that this will not break downstream code. For converters that use class paths these types of moves will break the converter.This PR changes the handling of class paths used for converters so that the 'public' path can be used. If a converter is not found for a class in
_converters_by_type(which now only contains classes as keys) then_converters_by_class_pathis indexed by checking what classes referred to by the paths are already imported (to avoid importing every class supported by every extension). If the class is imported it is added to_converters_by_typeand removed from_converters_by_class_path.Closes #1653
The changes in this PR will be helpful for maintaining
asdf_astropyand other asdf extensions that provide converters for types implemented in a different library. To illustrate this, the following branch was created on a fork ofasdf_astropythat uses the public class paths and when run with this PR passes all unit tests:https://github.com/astropy/asdf-astropy/compare/main...braingram:class_path?expand=1