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

Implement stringsSepBy1 parser #4

Merged
merged 1 commit into from Mar 6, 2017

Conversation

@rmunn
Copy link
Contributor

@rmunn rmunn commented Feb 21, 2017

This would fix #3.

I tried to write unit tests as well as implement the function, but I couldn't figure out how the existing stringsSepBy unit tests work in order to add a set of stringsSepBy1 tests (just like stringsSepBy but rejecting the case where the first parser doesn't match even a single time). Besides which, it wasn't immediately obvious to me how to get the code compiling on my Linux development box with VS Code + Ionide, so I couldn't have run the unit tests even if I had written them.

So I'm afraid you'll have to write the unit tests and make sure my code is correct before merging it in. But it's a very straightforward change, and I could copy the logic from manyStringsImpl just above. So I'm pretty sure that it's correct, I just couldn't prove it with unit tests.

@@ -1158,7 +1158,7 @@ val @many1Strings2@: Parser<string,'u> -> Parser<string,'u> -> Parser<string,'u>
val @stringsSepBy@: Parser<string,'u> -> Parser<string,'u> -> Parser<string,'u>
``]
[
`stringsSepBy sp sep` parses *zero* or more occurrences of `sp` separated by `sep`
`stringsSepBy p sep` parses *zero* or more occurrences of `p` separated by `sep`

This comment has been minimized.

@rmunn

rmunn Feb 21, 2017
Author Contributor

Note that in addition to documenting the stringsSepBy1 parser, I also fixed a small inconsistency in the stringsSepBy documentation.

@stephan-tolksdorf
Copy link
Owner

@stephan-tolksdorf stephan-tolksdorf commented Feb 21, 2017

Thank you for your pull request. I will have time to work on this towards the end of the next week.

@stephan-tolksdorf
Copy link
Owner

@stephan-tolksdorf stephan-tolksdorf commented Mar 5, 2017

Robin, the PR looks good! Could you add the stringsSepBy1 function to the fsi signature file, make stringsSepByImpl private and squash your commits? I will then merge the PR and add tests.

(I'm working with @neoeinstein on modernizing the VS solution and the build system, see #5, which will hopefully also make things easier for Ionide users.)

Also fix an inconsistency in docs: `p` should be `sp` in the
documentation of stringsSepBy (and ...By1) so that they match
the names used in the .fsi signature file.
@rmunn rmunn force-pushed the rmunn:stringsSepBy1 branch from a9c1fea to 99c7339 Mar 6, 2017
@rmunn
Copy link
Contributor Author

@rmunn rmunn commented Mar 6, 2017

Done. I also reversed my decision about changing the sp parser name to p in the stringsSepBy documentation: after adding stringsSepBy1 to the .fsi file, I noticed that you'd gone with a consistent naming scheme of using cp for char parsers and sp for string parsers. So instead, I fixed the places in the stringsSepBy documentation that were inconsistent with that scheme, and used sp for the docs of stringsSepBy1.

This should now be ready for you to merge and add the unit tests.

@stephan-tolksdorf stephan-tolksdorf merged commit de31c6c into stephan-tolksdorf:master Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.