Skip to content
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

Merged

Conversation

eslavich
Copy link
Contributor

This resurrects #1050 so that we can merge it to master and include in a 2.x release.

@eslavich eslavich requested a review from a team as a code owner January 21, 2023 09:05
@eslavich eslavich force-pushed the eslavich-custom-validators-2.x branch 2 times, most recently from 0000d1d to d2b558f Compare January 21, 2023 10:00
@eslavich eslavich force-pushed the eslavich-custom-validators-2.x branch from fcd253a to 1bf1b6d Compare January 21, 2023 10:11
@CagtayFabry
Copy link
Contributor

I will try to run this against the previous test branch again soon @eslavich

@WilliamJamieson
Copy link
Contributor

@eslavich is it possible to break this into smaller chunks?

Copy link
Contributor

@braingram braingram left a 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

asdf/core/_extensions.py Show resolved Hide resolved
asdf/core/_integration.py Show resolved Hide resolved
asdf/core/_validators/ndarray.py Show resolved Hide resolved
asdf/extension/_manager.py Show resolved Hide resolved
asdf/extension/_manager.py Show resolved Hide resolved
asdf/extension/_manager.py Outdated Show resolved Hide resolved
asdf/extension/_manager.py Outdated Show resolved Hide resolved
asdf/extension/_manager.py Show resolved Hide resolved
docs/asdf/extending/extensions.rst Show resolved Hide resolved
docs/asdf/extending/extensions.rst Show resolved Hide resolved
@braingram braingram added this to the 2.15 milestone Jan 26, 2023
@eslavich eslavich force-pushed the eslavich-custom-validators-2.x branch 2 times, most recently from f5b8740 to cc3bf69 Compare February 6, 2023 01:49
@eslavich
Copy link
Contributor Author

eslavich commented Feb 6, 2023

@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

@CagtayFabry
Copy link
Contributor

@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

Would it help to remove the code you linked from the new style extension to see if the legacy implementation still works @eslavich ?

@CagtayFabry
Copy link
Contributor

I have removed the incompatible validators from the new style extension, can you run the weldx downstream test again @eslavich ?

@eslavich
Copy link
Contributor Author

eslavich commented Feb 6, 2023

I have removed the incompatible validators from the new style extension, can you run the weldx downstream test again

Thank you! It looks like someone re-ran the test for me and it passed this time.

@braingram
Copy link
Contributor

@eslavich is it possible to break this into smaller chunks?

@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.

@braingram braingram force-pushed the eslavich-custom-validators-2.x branch from ead4788 to 382637a Compare February 9, 2023 20:34
@WilliamJamieson WilliamJamieson merged commit b3022f7 into asdf-format:master Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants