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

Better unit tests #33

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Better unit tests #33

wants to merge 6 commits into from

Conversation

Net-Mist
Copy link
Collaborator

@Net-Mist Net-Mist commented Apr 5, 2022

Hello !

When using the parser I found some bugs.
For now I wrote the failing corresponding unit-tests. The goal of this branch is to track the evolution of these tests and to be merged when they will all succeed.

Some of these bugs need correction in the lex/bison part, but other "issues" comes from the language definition and will (maybe) require to update the EBNF https://github.com/cooklang/spec/blob/main/EBNF.md

Regarding metadata:

  • users can't write stuff like >> source: https://www.gimmesomeoven.com/baked-potato/. Indeed according to the EBNF, the second ":" is not a valid char. I think we should allow it (and also other cooklang char)

Regarding number:

  • as discussed in Bug in integer parsing #19, parsing @thyme{20 000%springs} gives a quantity of 20. The multi-word should be correctly detected.

Regarding words:

  • recette de [croque]-monsieur is parsed as recette de . I think [ should be allowed if not followed by "-" OR an error should be raise if we want "[" to be a forbidden char
  • recette de croque->monsieuris parsed as recette de croque-monsieur. The > should be an authorized char if not followed by another > OR an error should be raised. Here the behavior of removing the char is very weird

Regarding comments:

  • meal for 4 people [- scale linearly -] and take 20 minutes to prepare [- does not scale-] is parsed as meal for 4 people. The text between the 2 comments are not parsed. This is also an error in the EBNF : it shouldn't be "[", "-", ? any character ?, "-", "]" but "[", "-", ? any character except "-" followed by "]" ?, "-", "]"
  • block comments can't be multi-line

@Net-Mist Net-Mist requested review from Seth-Harlaar and dubadub April 5, 2022 08:43
@Net-Mist Net-Mist mentioned this pull request Apr 5, 2022
@dubadub
Copy link
Member

dubadub commented Apr 5, 2022

Good catch.

I updated EBNF in regards to block comments cooklang/spec@f95d8e2.

users can't write stuff like >> source: https://www.gimmesomeoven.com/baked-potato/. Indeed according to the EBNF, the second ":" is not a valid char. I think we should allow it (and also other cooklang char)

I'm not sure I understand why do you think : is not allowed? Metadata value can contain text item which can include any punctuation character.

@Net-Mist
Copy link
Collaborator Author

Net-Mist commented Apr 5, 2022

Oh ! You're right, sorry about that.

As the test is failing, I think the error is somewhere else. Maybe the issue is not the ":" but the "/" char ? SYMB_CHAR doesn't seems to be included in the definition of Metadata in the lexer file :

METADATA              ">"">"{WHITE_SPACE}*({WORD}|{MULTIWORD})":"{WHITE_SPACE}*({PUNC_CHAR}|{WORD}|{MULTIWORD}|{NUMBER})*

@Seth-Harlaar
Copy link
Contributor

Seth-Harlaar commented Apr 5, 2022

I believe the parser fails because of the function that parses the metadata strings. Currently it is setup to use strtok to split the string up by the ':', which works fine when there is one. When there are multiple, strtok() removes them all from the string and causes the error.
I do have a fix that was going to be pushed on a different pull request that might have been rejected I believe. I do another pull request to fix this error.
This also comes with a change in the METADATA definition to be just:
METADATA ">"">"*
This will allow any characters in metadata.

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.

3 participants