-
Notifications
You must be signed in to change notification settings - Fork 50
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
Conversation
dd1b086
to
624ba68
Compare
@JordanMartinez @robertdp @thomashoneyman would one of you please review? |
624ba68
to
6d186aa
Compare
- `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.
6d186aa
to
736f863
Compare
There was a problem hiding this 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__. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for editing convenience.
There was a problem hiding this comment.
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?
Thanks for the review @robertdp |
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, 🤷♂️ 😄 |
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 combinescorrectly with
notFollowedBy eof
. Added a test for this.Also added a lot of documentation.
Resolves #25 #110 #100 #99 #68
Checklist: