Skip to content

Documentation and new parsers rest,take,eof #140

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 3 commits into from
Jan 13, 2022
Merged

Documentation and new parsers rest,take,eof #140

merged 3 commits into from
Jan 13, 2022

Conversation

jamesdbrock
Copy link
Member

@jamesdbrock jamesdbrock commented Jan 13, 2022

Description of the change

Add parsers:

  • Parser.String.rest
  • Parser.String.takeN
  • Parser.Token.eof

Bugfixes:

  • Parser.String.eof Set consumed on success so that this parser combines
    correctly with notFollowedBy eof. Added a test for this.

Also added a lot of documentation.

Resolves #25 #110 #100 #99 #68


Checklist:

  • Added the change to the changelog's "Unreleased" section with a link to this PR and your username
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation in the README and/or documentation directory
  • Added a test for the contribution (if applicable)

@jamesdbrock
Copy link
Member Author

@JordanMartinez @robertdp @thomashoneyman would one of you please review?

- `Parser.String.rest`
- `Parser.String.take`
- `Parser.Token.eof`

Bugfixes:

- `Parser.String.eof` Set consumed on success so that this parser combines
  correctly with `notFollowedBy eof`. Added a test for this.
Copy link
Collaborator

@robertdp robertdp left a comment

Choose a reason for hiding this comment

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

Looks good, thanks James.


Parsing requires two more capabilities: *choice* and *failure*.
Parsing requires two more capabilities: __alternative__ and __failure__.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "choice" -> "alternative" change makes the language fairly awkward. What was the reason for changing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because “alternative” is the term used by the Alt typeclass. For clarity I like to try to always use the same word when I'm talking about the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think "choice" is a more familiar word to use here because you clarify what you mean by "choice" in the next few paragraphs: Alt.

To run a parser, call the function `runParser :: s -> Parser s a -> Either ParseError a` in the `Text.Parsing.Parser` module, and supply it with an input string and a parser.
To run a parser, call the function `runParser :: s -> Parser s a -> Either ParseError a` in
the `Text.Parsing.Parser` module, and supply it with an input string and a parser.
If the parse succeeds then the result is `Right a` and if the parse fails then the
Copy link
Collaborator

Choose a reason for hiding this comment

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

How come there are all these extra line breaks in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just for editing convenience.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm curious why this was done too because it messes up the git blame. What does "editing convenience" mean?

@jamesdbrock
Copy link
Member Author

Thanks for the review @robertdp

@jamesdbrock jamesdbrock merged commit a0ab5bf into main Jan 13, 2022
@jamesdbrock jamesdbrock deleted the jbrock-docs branch January 13, 2022 13:46
@JordanMartinez
Copy link
Contributor

The added docs look good and definitely help clarify things. I also didn't know about the 'try considered harmful' paper, so I'd like to read that on my own time. So, in all, this is a net gain. That being said, I agree with the concerns @robertdp raised. But since this has already been merged, 🤷‍♂️ 😄

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