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

First pass at improving the Scaladoc comments. #37

Merged
merged 6 commits into from
Nov 4, 2020
Merged

Conversation

non
Copy link
Contributor

@non non commented Nov 4, 2020

I'm trying to work through and write more descriptive Scaladoc comments.

In particular, I've coined the term arresting failure to contrast with epsilon failure. I think this makes a lot of the prose easier to follow.

What do you think? cc @johnynek @rossabaker

@non
Copy link
Contributor Author

non commented Nov 4, 2020

(On reading it here I notice the prose is fairly clunky in some places. I plan to do another pass before I remove the "wip" title -- feel free to comment on any problems you notice but as far as typos or misspellings you can also wait until it's closer to ready.)

Copy link
Collaborator

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

Thanks. These comments are really nice!

I made a few comments.

* `x` returns an epsilon failure; these methods cannot recover from
* an arresting failure. Arresting failures can be "rewound" using
* methods such as `backtrack` or `softProduct`, which converts
* arresting failures into epsilon failures. Since rewinding is
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rewinding is actually cheap, but backtracking can result in exponential complexity and tends to make errors harder to locate.

* - Arresting failure: The parser fails to extract a value but does
* consume one-or-more characters of input. The input offset will
* be moved forward by the number of characters consumed. The
* consumed characters will not be available to any additional
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "will not be available" but isn't great wording IMO. After an arresting error, nothing more will be parsed, full stop. It will always be returned to the user.

* Operations such as `x.orElse(y)` will only consider parser `y` if
* `x` returns an epsilon failure; these methods cannot recover from
* an arresting failure. Arresting failures can be "rewound" using
* methods such as `backtrack` or `softProduct`, which converts
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't quite right about softProduct. SoftProduct makes a success in the first followed by an epsilon failure in the second into an epsilon failure of the whole thing. By contrast, a normal product the first may have consumed and thus you have an arresting failure even though the second item was an epsilon.

* This method will return a failure unless all of `str` is consumed
* during parsing.
*
* `p.parseAll(s)` is equivalent to `(p <* * Parser.end).parse(s).map(_._2)`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is an extra * before Parser.end.

* epsilon failure. The `?` method converts these failures into
* None values (and wraps other values in `Some(_)`).
*
* If the underlying parser failed with other errors, this parser
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to say "arresting error" here?

* When parsing an input string that the underlying parser matches,
* this parser will return the matched substring instead of any
* value that the underlying parser would have returned. It will
* still match exactly the same inputs as the original parser.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might mention this is very efficient and the same as .void.string internally.

*/
/**
* If this parser fails to match, rewind the offset to the starting
* point before moving on to other parser.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say "convert an arresting failure into an epsilon failure" at some point.

* Otherwise both extracted values are combined into a tuple.
*
* Backtracking may be used on the left parser to allow the right
* one to pick up after an error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't quite right. If A fails B can never run, the only question is will you have an epsilon or arresting failure. If A needs backtracking but B does not, you might do A.backtrack ~ B which is different from (A ~ B).backtrack

* current parser would fail
*/
/**
* Return a parses that succeeds (consuming nothing, and extracting
Copy link
Collaborator

Choose a reason for hiding this comment

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

S/parses/parser/

@non non changed the title [wip] First pass at improving the Scaladoc comments. First pass at improving the Scaladoc comments. Nov 4, 2020
Copy link
Collaborator

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

This is really great! Thanks for making the docs so much clearer.

@non non merged commit 124eb22 into main Nov 4, 2020
@non non deleted the topic/scaladoc-updates branch November 4, 2020 23:10
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.

None yet

3 participants