Skip to content

Broken satisfy #6

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

Merged
merged 5 commits into from
Jul 30, 2014
Merged

Broken satisfy #6

merged 5 commits into from
Jul 30, 2014

Conversation

nkly
Copy link

@nkly nkly commented Jul 30, 2014

Hello! While writing query string parser for my pet project I noticed that some simple parsers aren't working. I've digged a little and found that failing in satisfy actually consumes failed character and thus fails every other parser combinator that depends on satisfy. I've attached simple test and quite straightforward fix for this problem.

@paf31
Copy link
Contributor

paf31 commented Jul 30, 2014

I do agree that satisfy should not consume input if the predicate fails. However, I'd really like to not have to define this in terms of the underlying monad, but reuse things like char. I feel like we could fix this by just adding a try. Does that not work?

@nkly
Copy link
Author

nkly commented Jul 30, 2014

Yes, it's much better and clearer this way

@paf31
Copy link
Contributor

paf31 commented Jul 30, 2014

Does this work as you'd like? If so, I'll merge it.

@jdegoes You wrote this originally I think. Do you see any reason not to wrap this with try?

@jdegoes
Copy link
Contributor

jdegoes commented Jul 30, 2014

No, I'd prefer it with try. The new solution looks good to merge. @paf31?

paf31 added a commit that referenced this pull request Jul 30, 2014
@paf31 paf31 merged commit a198281 into purescript-contrib:master Jul 30, 2014
@paf31
Copy link
Contributor

paf31 commented Jul 30, 2014

Thanks 👍

@nkly nkly deleted the fix-satisfy branch July 30, 2014 18:09
@garyb
Copy link
Member

garyb commented Jul 30, 2014

Nice 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants