-
Notifications
You must be signed in to change notification settings - Fork 42
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
fix(input)!: Consolidate Input
traits
#107
Conversation
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.
clippy found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
This is modeled off of rust-bakery/nom#1612 with a mix of `combine::Stream. - Chose `Token`, like `combine`, as that is the term I've been generally standardizing on and is clearer in intent than `Item` - Baking in `Slice` support as that is important for wrapper types. I chose `Slice` over `combine`s `Range` as this is more focused on Rust integration, rather than grammar integration, and we are generally using `slice`. - `next_token` is really what some of nom's existing traits are trying to do but awkwardly. `combine` does this through `StreamOnce::uncons` - `iter_offsets` instead of `iter_tokens` as the only time you should care about the tokens, you should be looking at the offsets unless using something like `AsBytes`. - `next_slice` is a parallel to `next_token` but the return type differs as the lookup is done in a different function call. I'm tempted to add an `unchecked` variant but I want to benchmark first. I considered following `combine` and doing separate `InputToken` and `InputSlice` traits but figured we'd see how this goes first.
Couldn't touch `length_*` because it depends on other parsers
This also demonstrates a bug with the existing code
BREAKING CHANGE: trait bounds for `hex_u32` changed
Pull Request Test Coverage Report for Build 4068391968Warning: 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 |
This is modeled off of rust-bakery/nom#1612 and rust-bakery/nom#1610 with a mix of
combine::Stream
.Token
, likecombine
, as that is the term I've been generallystandardizing on and is clearer in intent than
Item
Slice
support as that is important for wrapper types. I choseSlice
overcombine
sRange
as this is more focused on Rust integration,rather than grammar integration, and we are generally using
slice
. This helped simplify things havingnext_slice
returnInput::Slice
rather than having to dointo_output()
.next_token
is really what some of nom's existing traits are trying to dobut awkwardly.
combine
does this throughStreamOnce::uncons
iter_offsets
instead ofiter_tokens
as the only time you should careabout the tokens, you should be looking at the offsets unless using something
like
AsBytes
.next_slice
is a parallel tonext_token
but the return type differs as thelookup is done in a different function call. I'm tempted to add an
unchecked
variant but I want to benchmark first.I considered following
combine
and doing separateInputToken
andInputSlice
traits but figured we'd see how this goes first.As part of this, a streaming bug in
hex_u32
was fixedBREAKING CHANGE: Input traits changed for most parsers and the old traits were removed