-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement support for extension URIs #850
Implement support for extension URIs #850
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing I would add is that I think these changes should be accompanied by updates to the developer documentation to indicate how things work after these changes, otherwise it will become very outdated.
@@ -247,55 +250,76 @@ def _check_extensions(self, tree, strict=False): | |||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize it is a tall order, but eventually it would be nice to have complicated methods like this have a docstring summarizing their purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add one to _check_extensions
and _process_extensions
now, since I just modified them.
tree_finalizer : callable, optional | ||
Callback that receives the tagged tree before it is validated | ||
and defaults are removed. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the description for the _serialization_context arguement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I omitted this intentionally, since it's not part of the public API. I didn't want to advertise it to external developers who might be using the method, since it's going to disappear in 3.0 anyway. Bad idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, perhaps a comment after the docstring should clarify the intent
asdf/type_index.py
Outdated
|
||
def _process_dynamic_subclass(self, custom_type): | ||
def _process_dynamic_subclass(self, custom_type, serialization_context): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhere the concept of dynamic subclasses should be explained. In my search of the code, I didn't easily find it. Perhaps better explained in the developer documentation; more on that later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little hazy on this myself, but I think the intention of this feature was to allow serialization of subclasses that didn't exist at the time that the TypeIndex
was constructed. When building the type index, we inspect the __subclasses__
attribute of each type and insert all the sub-types into a dict for fast lookup. But that only finds subclasses that have already been imported. Setting handle_dynamic_subclasses = True
causes the type index to make an actual issubclass
check against the listed types so it can catch subclasses that were imported later. It's slower but more comprehensive.
The new API doesn't support any brand of automatically serializing sub-types. A type must be explicitly listed in a converter in order to be serialized. So when TypeIndex
is dropped in 3.0 we won't have to worry about this import order issue anymore.
asdf/type_index.py
Outdated
@@ -163,17 +166,17 @@ def from_custom_type(self, custom_type): | |||
# includes classes that are created dynamically post |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like what exactly? There is no longer a _CompoundModel class, and no current subclasses of CompoundModel that I'm aware of. Or is this just to handle older versions of astropy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above, but I suspect the problem this was intended to solve was an import order issue. Without this feature, the following would succeed:
import SomeCustomCompoundModelSubclass
import asdf.type_index.TypeIndex
type_index = TypeIndex(...)
assert type_index.from_custom_type(SomeCustomCompoundModelSubclass) is not None
but this would fail:
import asdf.type_index.TypeIndex
type_index = TypeIndex(...)
import SomeCustomCompoundModelSubclass
assert type_index.from_custom_type(SomeCustomCompoundModelSubclass) is not None
3838377
to
73ace84
Compare
73ace84
to
15336c1
Compare
15336c1
to
9d2ba7e
Compare
I created #861 as a reminder to update the developer documentation before release. |
This PR is branched off of #849. Here I'm adding support for extension URIs, where are defined as an attribute on the extension instance and are written to the file metadata. Technically this is part of the new extension API, but the change fit nicely into a separate PR.
There are two new extension attributes:
extension_uri
: The URI of the "extension to the ASDF Standard" that the extension class implements. Something like "asdf://somewhere.org/extensions/foo-1.0".legacy_class_names
: The fully qualified names of extension classes that provided this functionality before the advent of URIs. This allows us to switch to URIs and change the class name without incurring warnings from the library.Additionally I changed
AsdfFile.__init__
andasdf.open
to accept extension URIs in addition to extension instances.