Add more context to schema parse errors #408
Conversation
github-actions
bot
added
area/api v0
| @@ -98,11 +98,12 @@ func TestEditCheck(t *testing.T) { | |||
| []*v0.RelationTuple{}, | |||
| []*v0.RelationTuple{}, | |||
| &v0.DeveloperError{ | |||
| Message: "parse error in `schema`, line 3, column 4: Expected identifier, found token TokenTypeRightBrace", | |||
| Message: "Expected identifier, found token TokenTypeRightBrace", | |||
There was a problem hiding this comment.
would this message be more readable with } instead of TokenTypeRightBrace?
There was a problem hiding this comment.
That's actually what I'm passing along now out of band from the error message itself. We could change the error message as well, but it would require a reverse map of the token types (which, I suppose would be fairly easy to do), but may appear odd for non-ASCII runes
There was a problem hiding this comment.
This could be a follow up, but it would it make sense to not keep a reverse map, but instead keep an "error message map"? then we can write the best representation for any given token.
There was a problem hiding this comment.
Yeah, we could do that too
There was a problem hiding this comment.
Let's do that as a followup
pkg/schemadsl/compiler/compiler.go
Outdated
| error: fmt.Errorf("parse error in %s: %s", formattedRange, errMessage), | ||
| SourceRange: sourceRange, | ||
| Source: input.Source(source), | ||
| error: fmt.Errorf("parse error in %s: %s", formattedRange, errMessage), |
There was a problem hiding this comment.
Isn't the thing you're doing here with error/BaseErrorMessage basically what you get with golang's error wrapping?
There was a problem hiding this comment.
Yes, but I wanted it to be explicit from the compiler's error type
There was a problem hiding this comment.
But whenever you wanted to get the underlying error, you can just errors.As(err, &BaseError{}) to get it back out. I'm not clear on the advantage to doing it outside the stdlib?
There was a problem hiding this comment.
Well, we'd have to declare another error type to do that. Do you prefer that approach?
There was a problem hiding this comment.
I think that would be more flexible, right? Then we could layer in other error types in the hierarchy if we needed
If you think it's more effort than it's worth this lgtm otherwise
josephschorr
force-pushed
the
better-schema-parse-errors
branch
from
0382172 to
dbc4f6c
Compare
No description provided.