Remove legacy extension api #1637
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
|
dkist also contains at least 1 ref by tag: Opened PR updating the schema to use a uri in the ref instead of a tag: |
braingram
force-pushed
the
remove_legacy_extension_api
branch
from
e428962
to
214f8be
Compare
braingram
force-pushed
the
remove_legacy_extension_api
branch
2 times, most recently
from
e428962
to
bc03ed4
Compare
braingram
changed the title
this prevents the BuiltinExtension from being loaded which means that the default tag_mapping that maps 'tag:stsci.edu:asdf' tags to uris is no longer included. This would be a breaking change so ``asdf.schema._tag_to_uri`` is added to map just these tags and issue an AsdfDeprecationWarning so we can remove this entirely in a later version
splitting up this commit would make the changes easier to understand. However the above change had numerous knock-on and dependent changes that required updating many tests and removing other deprecated functions/features. However, as everything removed was already deprecated this commit will exist in it's current overly large size.
braingram
force-pushed
the
remove_legacy_extension_api
branch
from
63cd7a1
to
fbcc1f9
Compare
5 tasks
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.
Builds on changes from:
Some notes so far:
BuiltinExtensionlegacy class name (so warnings should not be seen when a file is opened that was produced with the legacy extension api)$refusing a tag (not a tag uri) (see: Add warning when schema $ref is a tag #752)there are several uses in stdatamodels/jwst which should be updatedEDIT: stdatamodels uses fixed in: switch schema refs that use tag to uris spacetelescope/stdatamodels#201Closes: #752
Closes #1622
Deserialization types vs converters
The old type system caught TypeError raised on deserialization and turned it into a warning:
asdf/asdf/yamlutil.py
Lines 343 to 349 in 53b87b4
The new converters do not catch TypeError:
asdf/asdf/yamlutil.py
Lines 311 to 315 in 53b87b4
There were a few tests that relied on this behavior. However it appears that the
rawvalue is only passed on when aTypeErroris raised (all other exceptions bubble up and result in a failure to load the file). I see no documentation of this (looking at the 2.14.4 docs which have all descriptions of the old type system) and it seems like an inconsistent way to allow partial conversion failures during open. As we already have an open issue for discussing options for partial/graceful loading on failed conversions (#1555) I vote for not replicating this undocumented behavior of the type system and address the issue in a separate PR.