-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
error collection/recovery and reporting improvements #94
Conversation
@@ -186,19 +195,73 @@ impl<I: Stream + Location> FromRecoverableError<I, ContextError> for KdlParseErr | |||
} | |||
} | |||
|
|||
// This is just like the standard .resume_after(), except we only resume on Cut 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.
Didn't expect this to be more literally resume_after
but only for cuts. I take it FromRecoverableError
is working out? Any other input on the rest of the error recovery design?
FromRecoverableError
and Recoverable
just felt like they were getting messy and I wasn't too pleased with the results.
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.
honestly, once I figured out the right "surgical" cut I wanted (literally just resume_after
but for cuts), things started chugging along nicely and got a lot more intuitive.
The problem with the current resume_after
is that, a lot of times when I want to resume, it's when I specifically get an unambiguous syntax error ("I was definitely parsing a number, but I ran into something bad, and there's no way it could be anything else. abort abort"), but if it could be something else, I want my alt
s to keep working!
Besides that, FromRecoverable
and Recoverable
work... mostly fine, except it was a little confusing to figure out where error-related stuff should go (for example, you have to apply context before the resume_after
, otherwise the collected error never gets the context applied, which is obvious in hindsight but was a bit tricky to figure out. Docs would probably be good enough here). I'm sure they can be made simpler, but functionality-wise, they do what's needed, I think.
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.
The problem with the current resume_after is that, a lot of times when I want to resume, it's when I specifically get an unambiguous syntax error ("I was definitely parsing a number, but I ran into something bad, and there's no way it could be anything else. abort abort"), but if it could be something else, I want my alts to keep working!
Its easy for me to forget how much other people rely on alt
. In toml_edit
, I think my use if dispatch!
makes it so if I resume_after
in specific places, I don't have to worry about backtracking.
The intention behind including backtracking is to deal with making sure all errors that can be recovered are as I likely don't have good enough coverage in cut_err
to ensure resume_after_cut
will always work. I guess you could always cut_err
and resume_after_cut
. The docs would need to explicitly call out that workflow so people are less likely to overlook it.
Besides that, FromRecoverable and Recoverable work... mostly fine, except it was a little confusing to figure out where error-related stuff should go (for example, you have to apply context before the resume_after, otherwise the collected error never gets the context applied, which is obvious in hindsight but was a bit tricky to figure out. Docs would probably be good enough here). I'm sure they can be made simpler, but functionality-wise, they do what's needed, I think.
Thanks for that feedback!
cb4e3ad
to
5c7873b
Compare
0fbd06b
to
bbfc98c
Compare
bbfc98c
to
7c06ed0
Compare
Fixes: #93
the v2 parser can do this now: