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

Some general improvements / discussion #243

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

cpaulik
Copy link

@cpaulik cpaulik commented Feb 19, 2025

This adds three things

  1. Allows validation of extensions for collections.
  2. Improve error reporting by using best_match to select the error.
  3. Add a schema map argument similar to the node-stac-validator schemaMap to allow validating against local copies of schemas during development.

I'm happy to open separate MRs if one of the features is not wanted.
I'm also happy to add tests once we agree that a feature is going to be included.

@cpaulik cpaulik force-pushed the validate-collections-add-schema-map-improve-error-reporting branch from fb75fca to e37cf56 Compare February 19, 2025 12:52
@cpaulik cpaulik force-pushed the validate-collections-add-schema-map-improve-error-reporting branch from e37cf56 to acffbf1 Compare February 20, 2025 09:42
@cpaulik
Copy link
Author

cpaulik commented Feb 20, 2025

Force pushed a fix for the Mypy issues and updated existing tests as well.

@jonhealy1
Copy link
Collaborator

Hi @cpaulik all good ideas here :)

@cpaulik cpaulik force-pushed the validate-collections-add-schema-map-improve-error-reporting branch from d988288 to 7c60300 Compare February 20, 2025 13:32
@cpaulik
Copy link
Author

cpaulik commented Feb 20, 2025

Thanks. Tests should hopefully pass after the latest push. I forgot a breakpoint() in the code. 🤦

@cpaulik
Copy link
Author

cpaulik commented Feb 20, 2025

I'll add tests for the schema_map feature next. Still need to figure out if additional tests for the collection extensions functionality are needed. At least one of the tests I adapted seems to do that already.

cpaulik and others added 2 commits February 20, 2025 22:22
This also adds a schema property to make sure the validation message references
the overridden schema and not the original one
@cpaulik
Copy link
Author

cpaulik commented Feb 21, 2025

I've added another test for making sure references to sub-schemas also are replaced by the schema_map correctly.
So from my side this is done. Looking forward to the review.

@jonhealy1
Copy link
Collaborator

Hi @cpaulik. Looks really good. Just a couple of things. 1. An explanation and an example of a schema map, how to use them, to the readme. 2. An entry in the changelog, highlighting these changes. The changes would be added under the Unreleased section for now. Thanks!

@cpaulik
Copy link
Author

cpaulik commented Feb 25, 2025

Thanks, I added both. Let me know if that is what you had in mind.

@cpaulik
Copy link
Author

cpaulik commented Feb 25, 2025

I did just discover a small bug - stac-validator doesn't seem to validate extensions for collections if recursive mode is used. I'll need to look into that

@cpaulik
Copy link
Author

cpaulik commented Feb 25, 2025

That was just a matter of not keeping all the schemas in the message. That' fixed with the latest commit and handled similar to how it it done with item during recursive validation

@cpaulik
Copy link
Author

cpaulik commented Feb 25, 2025

I did just find another issue. It seems in recursive mode errors are not correctly reported since the default_validator doesn't actually raise a jsonschema.exceptions.ValidationError but just returns a message with valid_stac set to False.

I'll fix that as well.

@cpaulik
Copy link
Author

cpaulik commented Feb 25, 2025

The last commit should fix recursive reporting of errors for collections.

I do have a question about error reporting in the recursive case.
At the moment stac-validator reports all the messages. Wouldn't it be more useful to only show the messages that have valid_stac set to False in case of error?

At the moment in recursive mode we could print out 100s of messages that are valid which makes it hard to find the ones that actually cause the error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants