Skip to content

Use most specific class possible in schema validation errors#2883

Merged
joelostblom merged 5 commits intovega:masterfrom
binste:use_offending_class_for_schemavalidationerror
Feb 16, 2023
Merged

Use most specific class possible in schema validation errors#2883
joelostblom merged 5 commits intovega:masterfrom
binste:use_offending_class_for_schemavalidationerror

Conversation

@binste
Copy link
Contributor

@binste binste commented Feb 12, 2023

Addresses #2568 (comment)

Works already well but I'll still want to

  • Extend existing and add new tests to check if correct class name appears in error message
  • Test against multiple jsonschema versions (3, 4, 4.5, 4.16, 4.17)

…his is helpful in case error is based on e.g. StackOffset as a wrong parameter was passed to Y(stack='...') but what we actually want is channels.Y. Add tests
@binste binste changed the title Use class in which error occurred instead of highest class in SchemaValidationError Use most specific class possible in schema validation errors Feb 12, 2023
@binste
Copy link
Contributor Author

binste commented Feb 12, 2023

@joelostblom This already works if a wrong argument was passed to an encoding channel. Your example #2568 (comment) works anyway even without this PR because alt.Scale is immediately validated when it's instantiated. Do you have any other examples from #2568 I should test it against?

@joelostblom
Copy link
Contributor

This is looking great! Thanks for getting to it so quickly @binste. I tried a few different examples and they all seem to work fine. One thing that I did notice is that if we use to datum encoding then their message is incorrect:

alt.Chart().mark_point().encode(x=alt.datum(1, wrong_name=1))
SchemaValidationError: Invalid specification

        altair.vegalite.v5.schema.channels.X, validating 'additionalProperties'

        Additional properties are not allowed ('datum', 'wrong_name' were unexpected)

'datum' should not be included above as an incorrect property to the X channel, instead wrong_name should be mentioned as an incorrect parameter to datum. The way to handle this is probably with special error checking inside the datum function, and it is not introduced by this pull request, but I thought I would mention it here anyways. The tests here also look reasonable to me.

@binste binste marked this pull request as ready for review February 13, 2023 19:07
@binste
Copy link
Contributor Author

binste commented Feb 13, 2023

Good to go from my side. I can review #2568 if you want once you merged in these changes.

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