Skip to content
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

Provide shortcut for streaming::char #1410

Closed
wants to merge 1 commit into from
Closed

Conversation

epage
Copy link
Contributor

@epage epage commented Sep 27, 2021

The hope is that intent is clear enough while reducing syntactic noise.

The main ambiguity is streaming vs complete. I assume complete is
more common but streaming is the more universal one, so I went with
that.

The hope is that intent is clear enough while reducing syntactic noise.

The main ambiguity is `streaming` vs `complete`.  I assume `complete` is
more common but `streaming` is the more universal one, so I went with
that.
/// assert_eq!(parser(""), Err(Err::Incomplete(Needed::new(1))));
/// ```
fn parse(&mut self, i: I) -> IResult<I, char, E> {
crate::character::streaming::char(*self)(i)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Stargateur I figured streaming was a safe default Know of any reason to use complete or concerns with having to pick one over the other?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this could be a big problem... I guess this make impossible to do your idea. If you force streaming, this make any parser for complete not working and vice versa.

+ crate::traits::InputIter
+ crate::traits::InputLength,
<I as crate::traits::InputIter>::Item: crate::traits::AsChar,
E: crate::error::ParseError<I>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Stargateur

It looks like internal is a self-contained core of the crate, not depending on anything else.

  • Is it ok to use stuff from outside internal?
  • If not, should this impl live somewhere else, like next to char?

Once we've decided on how this should look, I'll clean this up and get it implemented for the other types discussed in #1408

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't speak for geal but I think it's ok but I would reverse, move code from character::char to here and call this implementation in character::char. Trait are ok but maybe parser is too much. Anyway, this file should probably be call parser anyway.

If not, should this impl live somewhere else, like next to char?

Since this impl Parser for a foreign type I think the correct place is here close to trait definition.

epage added a commit to epage/nom that referenced this pull request Sep 28, 2021
As part of rust-bakery#1410, the name `internal.rs` was discussed and it was
brought up that `parser.rs` would provide clearer intent.
epage added a commit to epage/nom that referenced this pull request Sep 28, 2021
As part of rust-bakery#1410, the name `internal.rs` was discussed and it was
brought up that `parser.rs` would provide clearer intent.
@epage epage closed this Sep 28, 2021
@epage epage deleted the impl branch September 28, 2021 14:35
epage added a commit to epage/nom that referenced this pull request Jan 14, 2022
As part of rust-bakery#1410, the name `internal.rs` was discussed and it was
brought up that `parser.rs` would provide clearer intent.
epage added a commit to epage/nom that referenced this pull request Jan 14, 2022
As part of rust-bakery#1410, the name `internal.rs` was discussed and it was
brought up that `parser.rs` would provide clearer intent.
epage added a commit to epage/nom that referenced this pull request Jan 14, 2022
As part of rust-bakery#1410, the name `internal.rs` was discussed and it was
brought up that `parser.rs` would provide clearer intent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants