Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ENH: Make the schema validation error for non-existing params more informative #2568
ENH: Make the schema validation error for non-existing params more informative #2568
Changes from 21 commits
555a6c2
430b17b
45f11b6
95b1c60
9bd738b
4eeac63
8dbf794
3a41601
f994245
bee7b8b
4395f58
8848228
1ce2bcf
31948e8
750bfd1
946ca90
8e9e7c7
7a89d76
01c517f
f9b4f3b
3718432
3022f8c
33296b0
b994959
afd614b
4e8bac5
187c817
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@binste I noticed an issue here after #2842 was merged. Previously, error messages would include a reference to the class where the error was raised, as in this case with the Y channel:
After #2842 the first part of that error message changes to no longer point to the particular class where the error was encountered, but just the chart in general:
In addition to being less informative in general, this becomes an issue here because this PR was relying on that first part of the error message to find all the correct parameters names that the Y channel accept. There might be ways to work around that in this PR, but I wanted to ask you first if you think it is possible to restore the more precise error message that points to the class where the error was encountered or if that information is lost with the new approach to raising errors.
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.
To add some more info, previously with deep validation, what I think was happening was that the iterative finding of the error meant that the class of the SchemaBase (I think) would change in each iteration to reflect which part of the spec was checked for errors. For example if I print out
type(self)
of theself
that is passed to theSchemaValidationError
it would look like this with deep validation (for a spec with a typo in a param in the Y channel):Without deep validation, all the errors are found without iteration and therefor the
self
param remains as the chart itself without changing to the Y channel where the error is occurring:I tried to look briefly if there was a way to not only return all the error messages, but also all the classes so that we could still find out that in this case it was the Y class that the error pertained to.
I looked briefly and it seems like some of this information is stored in the
absolute_path
attribute of theself
object inside theSchemaValidationError
class, and it might be possible to reconstruct it from there, but I couldn't find the full class path anywhere and I wanted to hear your thoughts before going down that path.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.
Thanks for the detailed write-up! Indeed, I agree that it would be more informative if that original behaviour is restored and of course helpful for your proposed changes here, which look very helpful.
I think
absolute_path
is the correct place to take this from. From that name, we can try to recreate the correct lass name. We can maybe use the usual conversion to a Python object name that is used in the propertysetter https://github.com/altair-viz/altair/blob/master/altair/utils/schemapi.py#L680 and also in the code generation https://github.com/altair-viz/altair/blob/master/tools/generate_schema_wrapper.py#L454:Places to modify would be in
SchemaValidationError.__str__
instead ofcls = self.obj.__class__
we could do something likeand before that in
SchemaValidationError.__init__
:I haven't tested this yet but most classes should be importable from the top-level like this right? For others we could still fall back on
self.obj
(theself
that is passed intoSchemaValidationError
).If I find more time in the next days I can make a PR with this to restore the original behaviour of showing the class in which the error occured. I could then also review this PR.
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.
See #2883
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.
Thank you! I merged in your PR and this seems to be working as intended now. There are a few failing tests which needs the error message updated; I think I can get to those on Friday or Sunday this week. There seems to be some larger error also where my solution here still needs some change to work with the new multiple returned error messages.