-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add recovery support to the parser #102
Comments
I had a go at trying to implement some recovery for a simple case where a key is being typed but the dependencies:
http:
pat // complete here
test: I don't know if I'm going about it the right way, but my current work is here: There's one thing in there that definitely doesn't feel right, which is stashing the current key node in a field. This is used to determine whether something the scanner believes is a
When allowing recovery, this would parse as (key: one, value: two) but I don't believe that's valid and was trying to detect this by comparing the indentation. Any pointers would be appreciated! (I can open as a PR if it would be better to iterate there). |
@munificent @natebosch are either of you familiar enough to provide pointers on the above? |
I'm not sorry. I haven't looked at this package in ages. @nex3 would be the resident expert, but I don't know if she ever works on this package anymore. I agree with @bwilkerson though that it would be good to have support for opt-in error recovery. |
I don't really work on this anymore, but I'm happy to consult. In this example:
I'm surprised that you're going for an error recovery that inserts a virtual indent rather than one that inserts a virtual colon. It seems easier, because you don't have to try to extend the block mapping context further than it would naturally go. |
Seems reasonable to me.. nit: It might be preferable to call it |
@nex thanks!
That's really what I was wanted to do, though I wasn't sure how to do it. I had another look and managed to simplify it - perhaps this is what you had in mind: Specifically: DanTup@8f1d54a#diff-c57d8918fb3309443f9d0b214d779552570b12e9f03e27f2ff99f4a97742ed99L489-R498 This covers a common case, although it falls down when the missing colon is on the first package: dependencies:
zero
one: any This one generates the following tokens (after I skipped this exception to recover):
This seems to happen because the indenting is greater than the I can file a PR if easier to comment on there. Thanks! |
I don't have a real opinion on the API so happy to change - although we specifically want to use this in "production" for the analysis server code completion so having to catch exceptions feels a bit awkward. It is already opt-in, but if it's too subtle the flag could be renamed or perhaps a separate function (for ex. |
Yeah, that might be attractive. I assume this will only be relevant for people doing code-completion and the likes. |
I'd much rather have a separate entry point than to throw an exception for non-exceptional behavior, though I honestly think that adding a named parameter would be just as effective at protecting current users from seeing changes. If we do have a separate entry point, let's name it something like |
Looking again, I do think an argument makes more sense too. There are already several entry points (
My current changes only recover from some fairly specific cases (half-typed lines without their colons). If it should always return something, there's more work to do. Would that be preferred to adding it gradually? |
That's a good point, I'm sold 👍 |
No. I'd take "always" as an aspirational goal at this point. |
I've opened a PR at #107 with the changes made so far if someone is able to review. |
We are using the YAML parser for development-time support, including code completion, and this requires the parser to be able to recover well in the face of errors in order to provide the best UX.
I believe that naively adding recovery would be a breaking change because the parser would begin to return guesses for what it thinks the user intended to type in situations where it previously would have returned
null
. I believe, however, that we can make it a non-breaking change by putting recovery behind a flag that is optionally passed to theloadYaml*
functions (something likebool recover = false
).Extending the API in this way would also allow us to incrementally implement recovery based on which failure cases occur most often.
Does that seem like a reasonable approach?
The text was updated successfully, but these errors were encountered: