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

Add EOF token to grammar #1162

Merged
merged 18 commits into from
Sep 26, 2023
Merged

Add EOF token to grammar #1162

merged 18 commits into from
Sep 26, 2023

Conversation

Lotes
Copy link
Contributor

@Lotes Lotes commented Aug 21, 2023

Adds the ability to explicitly parse the EOF token from Chevrotain.

@Lotes Lotes marked this pull request as ready for review August 21, 2023 14:26
@Lotes
Copy link
Contributor Author

Lotes commented Aug 22, 2023

@Yokozuna59 I had to discard the other PR. The current implementation only allows EOF within parser rules - so no terminals or terminal fragments.

I am wondering if it is still helpful. When I was searching for test examples, this EOF feature seemed not to be very powerful.

@Yokozuna59
Copy link
Contributor

Yokozuna59 commented Aug 22, 2023

The current implementation only allows EOF within parser rules - so no terminals or terminal fragments.

I am wondering if it is still helpful. When I was searching for test examples, this EOF feature seemed not to be very powerful.

@Lotes I think the main usage in my mind about it is to use it as EOL rule as @msujew said:

https://github.com/mermaid-js/mermaid/blob/6e0f41180fcb384df2188b77c93e8620a38d2e5d/packages/mermaid/src/diagrams/pie/parser/pie.jison#L84-L88

So it should be used as fragment rule:

fragment EOL:
  NEWLINE+ | ';' | EOF
;

The main issue we're facing is that each attribute should be in a separate line, so we can't use \s+ as hidden rule. But the last line mayn't end with \n, that why we override Lexer to add one:

https://github.com/mermaid-js/mermaid/blob/205360c1093ad0059989e05d5efecc4f019225b7/packages/parser/src/language/common/commonLexer.ts#L6

@Lotes
Copy link
Contributor Author

Lotes commented Aug 22, 2023

I think in your context that you have described, the new EOF keyword will help, so you are not dependent on the lexer hack. Since EOL is a parser rule, it will work.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looking pretty good, but there's a test case I'm missing (which is the main reason to use EOF at all): Can you create a test that builds a line based language and confirm that it parses as expected? I.e. something like this:

Lines: lines+=Line*;
Line: text=ID (EOL | EOF);
terminal EOL: /\r?\n/;

@Lotes
Copy link
Contributor Author

Lotes commented Sep 26, 2023

@msujew New tests were pushed and branch was rebased. Wingardium Approviosa? :)

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Alright looks good. Can you please remove the changes to the package-lock.json before merging?

@msujew msujew changed the title Add EOF(rebased) Add EOF token to grammar Sep 26, 2023
@msujew msujew added this to the v2.1.0 milestone Sep 26, 2023
@msujew msujew merged commit 5b02bfc into main Sep 26, 2023
3 checks passed
@msujew msujew deleted the lotes/eof2 branch September 26, 2023 13:54
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