-
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
deprecate ignore_version_mismatch #1819
deprecate ignore_version_mismatch #1819
Conversation
d3ea83b
to
ced39e8
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, one suggestion for a slightly more explicit warning message
8366d39
to
7ab3183
Compare
Thanks for taking a look. Unfortunately the jwst PR was merged :-/ That's my bad for not waiting to open that one. It includes a warning ignore that matches on the message currently used in this PR. We could:
Or if you think the message is "good enough" we could go ahead with the existing message. We'd still have to wait for the stdatamodels PR so making another jwst PR doesn't seem like an issue. Would you like me to update the warning filter in jwst? |
nah, I think this message is good enough! |
7ab3183
to
9103cb9
Compare
9103cb9
to
0e7ce93
Compare
This PR deprecates
ignore_version_mismatch
. Since asdf 3.0.0 it has done nothing (it was only used for the legacy extension API which was removed in asdf 3.0.0).There are a few downstream uses in stdatamodels mostly in tests. However there is one jwst test that fails due to a non-specific warning filter:
https://github.com/spacetelescope/jwst/blob/89f126a035751674f29a9027e2deae11b3e63fca/jwst/assign_wcs/tests/test_schemas.py#L57
Requires spacetelescope/stdatamodels#313
and likely spacetelescope/jwst#8693 (or a release of stdatamodels with the above PR)
before merging. I'm opening it for review now in case there is discussion about the warning message (changes to the message would require updating the jwst PR).
Closes #1812
Checklist: