-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add new plugin type for custom schema validators #1328
Add new plugin type for custom schema validators #1328
Conversation
0000d1d
to
d2b558f
Compare
fcd253a
to
1bf1b6d
Compare
I will try to run this against the previous test branch again soon @eslavich |
@eslavich is it possible to break this into smaller chunks? |
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.
Overall this looks great.
I made a few minor documentation suggestions, changed some imports to relative and highlighted one test coverage gap (because of the change in ndarray validator scope).
Edited: removed the relative imports per Williams objection
f5b8740
to
cc3bf69
Compare
@braingram I think I've addressed all of your comments. The weldx failures are due to them using the old-style validators in their new-style extension: https://github.com/BAMWelDX/weldx/blob/master/weldx/asdf/extension.py#L62-L66 As for stdatamodels, the same tests fail on asdf master so I don't think they're related to the changes in this PR: https://github.com/asdf-format/asdf/actions/runs/4099643240/jobs/7069745475 |
Would it help to remove the code you linked from the new style extension to see if the legacy implementation still works @eslavich ? |
I have removed the incompatible validators from the new style extension, can you run the weldx downstream test again @eslavich ? |
Thank you! It looks like someone re-ran the test for me and it passed this time. |
@WilliamJamieson do you have a proposal for breaking this up or are you happy with it as is? If you're happy with it I will rebase and merge this PR. |
ead4788
to
382637a
Compare
Co-authored-by: Brett Graham <[email protected]>
Co-authored-by: Brett Graham <[email protected]>
for more information, see https://pre-commit.ci
382637a
to
6e99e59
Compare
This resurrects #1050 so that we can merge it to master and include in a 2.x release.