-
Notifications
You must be signed in to change notification settings - Fork 277
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
[JSONSelection] Report better parsing error messages using extra: X
field of LocatedSpan<T, X = ()>
#6089
[JSONSelection] Report better parsing error messages using extra: X
field of LocatedSpan<T, X = ()>
#6089
Conversation
CI performance tests
|
...ots/apollo_federation__sources__connect__json_selection__parser__tests__error_snapshots.snap
Outdated
Show resolved
Hide resolved
39da235
to
4e30de5
Compare
4e30de5
to
4d96927
Compare
✅ Docs Preview ReadyNo new or changed pages found. |
1cf229b
to
506a282
Compare
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.
I finally tried this out and it works great! One suggestion on the API that will make using these improved errors a bit easier elsewhere (but requires updating consumers now since the API changes). I'm happy to make a PR into this which updates validation & the like if that'd be helpful.
The LocatedSpan<T, X = ()> type supports an optional user-defined `extra: X` field, which we can use to smuggle meaningful custom error messages out of the parser.
506a282
to
76949bd
Compare
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.
Looks great, thanks! I'll get it wired up to show the offsets in the LSP in a follow-up.
// ParseResult is the internal type returned by most ::parse methods, as it is | ||
// convenient to use with nom's combinators. The top-level JSONSelection::parse | ||
// method returns a slightly different IResult type that hides implementation | ||
// details of the nom-specific types. | ||
// | ||
// TODO Consider switching the third IResult type parameter to VerboseError | ||
// here, if error messages can be improved with additional context. | ||
pub(super) type ParseResult<'a, T> = IResult<Span<'a>, T>; |
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.
In the future (not suggesting you need to go and change the ones in this PR), if you use triple-slash quotes right before an item, it sets that comment as the docs for the item. Docs also support Markdown syntax, so you can get nicely-rendered docs for things in your editor. Here's in RustRover, where that link to JSONSelection::parse
actually brings you to the right code!
The
LocatedSpan<T, X = ()>
type provided by thenom_locate
crate (first introduced in PR #5987) supports an optional user-definedextra: X
field, which we can use to smuggle meaningful custom error messages out of the parser.It would be ideal if we could report multiple parsing errors and continue best-effort parsing after an error occurs, but that will have to wait for future PRs. Also, there are some cases where parsing fails because various
alt(...)
combinators ran out of options, and we can't (yet) determine which underlying error was "most" responsible for the failure, so the error message remains an opaquenom::error::ErrorKind
code.As a side benefit, I've attempted to stabilize the method signature of the public
JSONSelection::parse
method, so consumers don't depend onnom
- andnom_locate
-specific types, so we have the freedom to swap out implementation details in the future without breaking consumers. This primarily means continuing to accept a&str
as input (rather than aLocatedSpan
) and returning a customJSONSelectionParseError
type in case of failure.While improving parsing error messages is still a work in progress, I think this PR meaningfully improves the situation, and is worth considering as a first step.