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

fix(validate): fix order of fields in validation errors and tweak line placement #140

Closed
wants to merge 1 commit into from

Conversation

mildwonkey
Copy link

These are purely aesthetic changes to error messages returned by Validate()

Two changes in here:

  • The "expected" and "contained" values in the "data is not an instance" errors were swapped; fixed.
  • moved around the newlines in "no Thema handler for CUE error" so it was clear which error message that warning goes with. The placement of the newlines made it look like that message was following the previous error, instead of referring to the following error.

Before - note that the "no Thema handler" lines are part of the error message below the newline

<dashboard@v0.1>._sortedSchemas.1._#schema.gnetId: validation failed, data is not an instance:
	schema expected `null`
		dashboard.json:18:13
		/scthema/dashboard_kind.cue:571:13
	but data contained `string`
		/scthema/cue.mod/pkg/github.com/grafana/thema/lineage.cue:258:20
no Thema handler for CUE error, please file an issue against github.com/grafana/thema
to improve this error output!

lineage._sortedSchemas.1._#schema.panels.0: 3 errors in empty disjunction:
no Thema handler for CUE error, please file an issue against github.com/grafana/thema
to improve this error output!

lineage._sortedSchemas.1._#schema.panels.0.animationModes: field not allowed

After:

<dashboard@v0.1>._sortedSchemas.1._#schema.gnetId: validation failed, data is not an instance:
	schema expected `string`
		dashboard.json:18:13
		/scthema/dashboard_kind.cue:571:13
	but data contained `null`
		/scthema/cue.mod/pkg/github.com/grafana/thema/lineage.cue:258:20
no Thema handler for CUE error, please file an issue against github.com/grafana/thema
to improve this error output:
lineage._sortedSchemas.1._#schema.panels.0: 3 errors in empty disjunction:


no Thema handler for CUE error, please file an issue against github.com/grafana/thema
to improve this error output:
lineage._sortedSchemas.1._#schema.panels.0.animationModes: field not allowed

I left the newlines after the "no Thema handler" message, but moved them to after the error they were referencing; I think we would be better off removing all the extra newlines but I opted to stay closer to the existing implementation. I'm happy to change that.

I added incredibly sad and minimal tests, nothing I'd write a full test suite - just enough to illustrate the changes.

…line placement

Two changes in here:
* The "expected" and "contained" values in the "data is not an instance" errors were swapped; fixed.
* moved around the newlines in "no Thema handler for CUE error" so it was clear which error message that warning goes with. The placement of the newlines made it look like that message was following the previous error, instead of referring to the following error.
@mildwonkey mildwonkey requested a review from sdboyer May 9, 2023 18:06
@sdboyer
Copy link
Contributor

sdboyer commented May 10, 2023

I added incredibly sad and minimal tests

still less sad and minimal than what was there before - literally an empty validate_test.go file?!? wow, past sam, wow 😳😳

The reason this oddity has been hanging around awkwardly for so long is that the problem only occurs sometimes - which your test cases seem to illustrate. Swapping the output fields causes this case to misidentify 42 as the schema and bool as the data.

What's behind this confusing behavior is the crappy heuristic i used to extract information from the underlying CUE errors. That logic is so incomplete because thema has historically had such a poor corpus of test cases - something we're now finally making headway on. That, and because the CUE errors package lacks clear abstractions that make programmatic analysis easier.

So i'm torn on this PR - while i want to see an improvement in this output, it's not clear to me that just swapping the outputs is worth doing, because we make one set of cases correct, and a different set incorrect.

@sdboyer
Copy link
Contributor

sdboyer commented May 10, 2023

i am noticing _sortedSchemas.1._#schema.gnetId in your output here though, and realize that this is a thing i forgot to fix up in lineage flattening. Will fix that quickly...

Also, that message about filing an issue against thema is clearly pointless and should just be removed. But again, not this PR.

@mildwonkey
Copy link
Author

The reason this oddity has been hanging around awkwardly for so long is that the problem only occurs sometimes - which your test cases seem to illustrate. Swapping the output fields causes this case to misidentify 42 as the schema and bool as the data.

... well now I'm embarrassed I didn't see that! I'll close this and we can open an issue to fix this ~for reals; I didn't realize those variables weren't reliably holding the "got" vs "want" values.

@sdboyer
Copy link
Contributor

sdboyer commented May 10, 2023

sounds good!

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.

None yet

2 participants