feat: Add output_stream/with_output_stream to Parser
#233
Conversation
This is a draft implementation to solve winnow-rs#231
|
@epage any opinion about this? |
| /// Err(ErrMode::Backtrack(Error::new(BStr::new(";"), ErrorKind::Verify))), | ||
| /// ); | ||
| /// ``` | ||
| fn output_stream(self) -> OutputStream<Self, I, O, E> |
There was a problem hiding this comment.
I think I prefer recognize, rather than output as that does clarify what the stream's range is
There was a problem hiding this comment.
Yep, recognize_stream also feels more fitting to me. Will rename it when I polish the PR.
| match self.parser.parse_next(input.clone()) { | ||
| Ok((remaining, _)) => { | ||
| let offset = input.offset_to(&remaining); | ||
| let (remaining, slice) = input.next_slice(offset); | ||
| Ok((remaining, input.update_slice(slice))) | ||
| } | ||
| Err(e) => Err(e), |
There was a problem hiding this comment.
Should the streams be marked complete?
There was a problem hiding this comment.
Maybe I'm missing something, but how would I mark a stream as complete?
There was a problem hiding this comment.
| @@ -212,29 +216,29 @@ pub trait Parser<I, O, E> { | |||
|
|
|||
| /// Produce the consumed input with the output | |||
| /// | |||
| /// Functions similarly to [recognize][Parser::recognize] except it | |||
| /// returns the parser output as well. | |||
| /// Functions similarly to [`recognize`][Parser::recognize] except it returns the parser output | |||
There was a problem hiding this comment.
Unrelated changes will need to be reverted
There was a problem hiding this comment.
Can do that, but I thought it would make sense to add the missing backticks around recognize as part of this PR to align it with the docs of the new methods since they are related.
There was a problem hiding this comment.
If so, that should be a separate commit
| /// Err(ErrMode::Backtrack(Error::new(BStr::new(";"), ErrorKind::Verify))), | ||
| /// ); | ||
| /// ``` | ||
| fn output_stream(self) -> OutputStream<Self, I, O, E> |
There was a problem hiding this comment.
Could length_value be updated to use this?
There was a problem hiding this comment.
I'll have a look.
| /// consumed_parser.parse_next(BStr::new("abcd")), | ||
| /// ); | ||
| /// ``` | ||
| fn with_output_stream(self) -> WithOutputStream<Self, I, O, E> |
There was a problem hiding this comment.
Have you considered trying these out on an extension trait in your crate first to explore their usage?
I am considering doing 0.5 in the next couple months, so we do have room to change things soon if we aren't thrilled with it. I'm just exploring (and reminding myself) what the options are for these
There was a problem hiding this comment.
I didn't consider that yet, no. But it's a great idea and I'll do this to see if the additional methods are really that useful as I imagine. Will report back once I did that.
I already played around with output_stream in my hcl-edit parser by pointing it against my local winnow fork and it actually helped to simplify one of my use cases quite a bit. Didn't check how (if) with_output_stream would help simplifying the parsers that receive a state parameter yet, but I did a small PoC in a bigger unit test that I didn't bundle with the PR yet to verify that state updates within a map operation actually work the way I expect.
|
@epage I think the new |
Pull Request Test Coverage Report for Build 4722337009Warning: 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 a draft implementation to solve #231
As noted in the initial issue, I'm not too settled on the naming here, there are other alternatives like
recognize_stream/with_recognized_stream.If you think the idea is worth pursuing further, I can also add some tests/examples for use cases like the following:
Parser::output_stream().and_then(..)to map another parser over theStream.Parser::with_output_stream().map(...)to access/update thestatein aStatefulin the map closure.Happy to hear your thoughts on this.