mrkkrp / parser-combinators

Lightweight package providing commonly useful parser combinators
Other
52 stars 15 forks source link

Behaviour of someTill is confusing. #25

Closed kindaro closed 4 years ago

kindaro commented 4 years ago

manyTill p end applies parser p zero or more times until parser end succeeds. ... someTill p end works similarly to manyTill p end, but p should succeed at least once.

(From the haddock)

Nothing in the above prepares me to this:

λ parseTest @Void (manyTill (anySingle) (single '!')) "any chars ! wrong chars !"
"any chars "
λ parseTest @Void (someTill (anySingle) (single '!')) "any chars ! wrong chars !"
"any chars "
λ parseTest @Void (manyTill (anySingle) (single '!')) "! wrong chars !"
""
λ parseTest @Void (someTill (anySingle) (single '!')) "! wrong chars !
"! wrong chars "
  1. What I read is this:

    someTill p end applies parser p zero or more times until parser end succeeds, but p should succeed at least once.

  2. What I see is this:

    someTill p end applies parser p one or more times until parser end succeeds.

Indeed, that is how it is defined:

someTill p end = liftM2 (:) p (manyTill p end)

I am not sure if it is not too late to change this behaviour, but I at least wish for it to be spelled out.

mrkkrp commented 4 years ago

A haddock update would be in order here. Want to open a PR?

kindaro commented 4 years ago

Sure, but how about we actually provide functions that work the way they are supposed to? Maybe under a different name?

mrkkrp commented 4 years ago

that work the way they are supposed to

How those function would behave? In your example the result is totally reasonable imo and only appears sightly surprising because anySingle overlaps with any other parser consuming single character, such as single '!'.

kindaro commented 4 years ago

As the description says. Apply parser p zero or more times until parser end succeeds, but make sure p succeeded at least once. That is, try end before p in the first iteration as well as the latter.

In other words, someTill should parse exactly the same strings as manyTill except for those that have 0 occurrences of p. If manyTill would not consume some input, someTill should not either. The "some" qualification here is a restriction, not a relaxation.

mrkkrp commented 4 years ago

I think I understood what you mean, although it took some pondering. I think however that the present behavior is intuitive enough and it's de facto how these combinators were defined even long ago before this package came into existence. I don't think we can change it.

Also, important thing about this sort of library, and many APIs in general is that we should not define shortcuts for absolutely everything. In this case AFAIU, you can just do:

foo = ("" <$ end) <|> someTill p end

And this literal expression is probably easier and quicker to understand than yet another "special case" combinator.

kindaro commented 4 years ago

Important thing about this sort of library, and many APIs in general, is also that we should provide code that is tested throughout, robust and safe to use, rather than prod the user to patch our shortcomings and eventually get hurt by a subtle inconsistency such as this one. While not insisting that we change something or even extend the API surface, I do think we should take this as a lesson.

Anyway, how do you think it is best to fix the descriptions? Should we include your code example right there?

I do not mean to be offensive in any way.

mrkkrp commented 4 years ago

how do you think it is best to fix the descriptions

I think in this case just showing how someTill is defined (using haddock's > ... block) is the easiest way to clarify the behavior, especially that the definition is one-liner.

mrkkrp commented 4 years ago

Resolved in 4262c00ef70cc30bfc56db0a10c37d88ad88fe1a.