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

fix #47 P.either should fail with fatal error, if second parser faile… #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

regevbr
Copy link

@regevbr regevbr commented Jul 7, 2021

This is a bit of a "big" PR, so I can split it if necessary.

After going through the entire library for educational purposes, I have came about many things I wanted to add/fix/test and I have came up with the following:

  • added charC - a case insensitive char parser.
  • added stringC - a case insensitive string parser.
  • added oneOfC - a case insensitive one of strings parser.
  • Added non breaking change support for the between parser to be able to provide 2 different left and right parsers
  • Fixed the semigroup to mark as fatal if any was fatal and not if just the first one
  • Obviously added tests to all additions/changes, maintaining 100% coverage
  • Added some missing tests here and there
  • Enhanced the command example
  • Enhanced small bits of code here and there

@gcanti please let me know how do you want to proceed here, I would appreciate any feedback :-)

@IMax153 IMax153 requested a review from gcanti August 18, 2021 11:33
@IMax153
Copy link
Collaborator

IMax153 commented Aug 18, 2021

Hey @regevbr - thank you very much for your work on this PR!

However, in general, I would suggest that PRs be kept highly focused on whatever issue is being addressed. Otherwise it can be quite difficult to tease apart what the actual purpose of the PR was.

Could you please trim down the PR to address only your suggestion relating to the ParseError module?

In addition, I'm still not 100% sure if the semantics of using semigroupAny here is correct so I do think having @gcanti weigh in would be helpful.

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.

2 participants