feat(error)!: Provide a simple, getting started error #282
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
With the new design, this is not a general purpose error to recommend but is more helpful for writing tests of generic parsers to easily verify the failure location.
This is modeled off of `combine` and what I do for errors in `toml_edit` The main thing lacking is merging it with the input on `Parser::parse`. Performance is good: ``` json/basic/canada time: [9.9270 ms 9.9518 ms 9.9784 ms] json/unit/canada time: [9.0616 ms 9.0910 ms 9.1183 ms] json/verbose/canada time: [31.558 ms 31.722 ms 31.893 ms] json/context/canada time: [12.650 ms 12.679 ms 12.710 ms] ```
This will provide more capabilities out of the box, leading to a better user experience. For people who want little error context but high performance, they can switch the error type.
`ParseError` is a holdover from when it was being returned by `Parser::parse` but now it is being returned by `Parser::parse_next` or `Parser::parse_peek`. The new name is to convey this is what the parser is returning. This is opening room for a `ParseError` which will be a concrete type that wraps `ParserError` when returned by `Parser::parse`.
`VerboseError` was significantly slower (3x parse time) and awkward to work with (requiring extra input to render). Instead, `Context` error serves most purposes and users can write their own.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Pull Request Test Coverage Report for Build 5490674182
💛 - Coveralls |
epage
force-pushed
the
error
branch
3 times, most recently
from
f9f8913
to
4811ca9
Compare
Pull Request Test Coverage Report for Build 5490674182Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The default errors for
winnowwere a bad experience, making it so the default parameter forPResult/IResultwas just there for quick prototyping.This changes the error to mirror what is inside of
toml_edit. It also leverages the work done in #72 to remove generic parameters so it could be the default error forPResult.This also adjusts
Parser::parseto automatically add relevant context back into the error.Users can use the built-in
Displayor they can provide their own conversion function.Note:
VerboseErrormade parsing take 3x as long. The newContextErrormakes parsing take 1.1x as long or so.Fixes #103