-
Notifications
You must be signed in to change notification settings - Fork 57
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
Resolve external references in custom schemas #738
Resolve external references in custom schemas #738
Conversation
Codecov Report
@@ Coverage Diff @@
## master #738 +/- ##
==========================================
+ Coverage 93.3% 93.31% +<.01%
==========================================
Files 39 39
Lines 4391 4382 -9
==========================================
- Hits 4097 4089 -8
+ Misses 294 293 -1
Continue to review full report at Codecov.
|
|
||
if custom_schema is not None: | ||
self._custom_schema = schema.load_custom_schema(custom_schema) | ||
schema.check_schema(self._custom_schema) | ||
self._custom_schema = schema.load_schema(custom_schema, self.resolver, True) |
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.
This was moved after the call to self._process_extensions so that we could use our AsdfFile's resolver when loading the custom schema.
@@ -1,9 +1,7 @@ | |||
%YAML 1.1 | |||
--- | |||
$schema: "http://stsci.edu/schemas/yaml-schema/draft-01" | |||
id: "http://stsci.edu/schemas/asdf/core/asdf-1.1.0" |
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.
Re-using the core schema id causes problems when resolving external references. I don't think it's a good idea anyway, since the ids are meant to be globally unique.
custom = load_schema(url, resolve_local_refs=True) | ||
core = load_schema(AsdfObject.yaml_tag) | ||
|
||
def update(d, u): |
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 don't think it's actually necessary to merge the core schema with the custom schema, since we already check against the core schema in a first validation pass.
423d033
to
a6d817a
Compare
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.
LGTM
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.
Looks great!
Just a couple niggles below that should not prevent from merging.
Previously, external references in custom schemas were not being resolved (see #683). This was clearly deliberate, though I can't think of a reason why that would be the desired behavior. This PR changes the custom schema behavior to resolve external references, and fixes an issue in the tests that prevented the example custom schema from resolving correctly.
Resolves #683