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

Allow multiple expected values to be passed into desc and makeFailure calls #256

Merged
merged 4 commits into from
Jun 16, 2018

Conversation

MrJohz
Copy link
Contributor

@MrJohz MrJohz commented Jun 14, 2018

There are certain parsers that will accept multiple strings/characters, but only return one expected value - most parsers created through the use of Parsimmon.test() fall into this category. For example, oneOf('+-%') returns 'expected a character/byte matching function [...]', whereas in most cases it should probably return expected one of '+', '-', '%'. Especially when you have parsers that include branches, and description arrays are merged, it can become helpful to be able to specify arrays in .desc() and makeFailure()

I've made small changes to the .desc() and makeFailure() functions, and added tests to cover both of these changes. (None of the existing tests needed changing, although I did move the existing makeFailure test into a suite given that there are now multiple tests for that function.) I've also made changes to the API.md file to reflect the addition to the API.

I believe the changes are backwards compatible, and constitute a minor semver change.

It might also be worth at this point changing oneOf() to modify the description of the returned parser. I wanted to make these changes because the 'matching function blah' error message is not very helpful. However, that arguably constitutes an additional change, and is probably closer to breaking backwards compatibility, at least if people rely on the output of the error message, so I haven't done it in this PR, but I can add it if you want.

@coveralls
Copy link

coveralls commented Jun 14, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 7a9cee0 on MrJohz:master into 6690c37 on jneen:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f99afd1 on MrJohz:master into 6690c37 on jneen:master.

@wavebeem
Copy link
Collaborator

Hmm, interesting. I have thought about this before but it didn't really seem worth the change to me. I guess I never really saw one parser as having multiple expectations, so it seems kind of backwards to me. Also because generally adding too many .desc(...) causes problems with error reporting. But this does feel totally sensible and I can't see any reason to turn it down 😄

That being said, the error message behavior of oneOf(...) is totally broke and thanks for pointing that out. I can see now that a function is being concatenated into the error message, oops: https://github.com/jneen/parsimmon/blob/master/src/parsimmon.js#L920 😅

I have purposefully avoided testing the specific error messages from the built-in parsers because I do not consider them part of the API as much as they are developer messages, and should be updated to reflect the most useful message they can. 👩‍💻

That message in particular has already been updated as it used to only say a character not a character/byte, so if you want to fix that here too it would definitely be appreciated. 🙏

@wavebeem
Copy link
Collaborator

Oh and also, thank you very much for your first time contribution to Parsimmon! I'm always happy to see new folks happy to improve the library.

@MrJohz
Copy link
Contributor Author

MrJohz commented Jun 15, 2018

I've changed oneOf() to split the string into individual characters and report those as the description. I also changed noneOf(), because that also uses test() without giving a more explicit description, but I figured it's probably best to not split those chars, because it's not (not "a") or (not "b") but more (not "a" and not "b").

@wavebeem
Copy link
Collaborator

Sounds good. Hopefully I'll get a chance to merge this, update the changelog, and release soon :)

@wavebeem
Copy link
Collaborator

everything looks fine but the node 7 travis job failed for reasons i can't figure out… i'll try running the job a couple more times just in case

@wavebeem
Copy link
Collaborator

hmm, I just realized a really weird thing:

https://github.com/MrJohz/parsimmon/blob/7a9cee0e578160db28a378e5cf2a335538f8b3b0/src/parsimmon.js#L294-L298

i'll make a quick change to stop using that helper since it's no longer safe :)

@wavebeem wavebeem merged commit 11fb182 into jneen:master Jun 16, 2018
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