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 comment inside descriptions #334

Merged
merged 18 commits into from
Jan 23, 2025

Conversation

jacob-wieland-gematik
Copy link
Contributor

@jacob-wieland-gematik jacob-wieland-gematik commented Dec 9, 2024

🤔 What's changed?

⚡️ What's your motivation?

Fixes: #333

🏷️ What kind of change is this?

  • ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

📋 Checklist:


This text was originally generated from a template, then edited by hand. You can modify the template here.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Dec 9, 2024

The parser is generated. To change the parser you'll have to change the grammar.

gherkin/gherkin.berp

Lines 35 to 38 in 00fcc43

// we need to explicitly mention comment, to avoid merging it into the description line's #Other token
// we also eat the leading empty lines, the tailing lines are not removed by the parser to avoid lookahead, this has to be done by the AST builder
DescriptionHelper := #Empty* Description? #Comment*
Description! := #Other+

To regenerate the parsers you can follow these instructions:

https://github.com/cucumber/gherkin/blob/main/CONTRIBUTING.md#generating-parsers

@jacob-wieland-gematik
Copy link
Contributor Author

I have changed the grammar to allow any mixture of empty lines, comments and other-lines as the description.

Extracting only the other-lines from the Description-AST-Node (as before) should yield the description-text without the comments and empty lines.

Only difference is now that a collection of ony empty-lines and comments would yield an empty description text instead of a non-existing Description node.

@mpkorstanje mpkorstanje self-requested a review December 10, 2024 18:12
@jacob-wieland-gematik
Copy link
Contributor Author

Changed the grammar in a way that is more consistent with the former test cases (treating empty lines inside description blocks as '#Other' lines).

@kieran-ryan
Copy link
Member

@jacob-wieland-gematik, installing berp on the test-codegen workflow and running the tests on the test-dotnet workflow are failing owing to incompatibilities with ubuntu-24.04 which has now been promoted as the ubuntu-latest GitHub runner and is being picked up in the pipeline. I have pinned those workflows to ubuntu-22.04 for now (#348); and you may want to pull in those changes to allow those workflow steps to proceed.

@jacob-wieland-gematik
Copy link
Contributor Author

@kieran-ryan I have rebased on latest main and updated a tokens file. Can you approve the checks again?

@jacob-wieland-gematik
Copy link
Contributor Author

Added dialect.c which was missing for some reason. Please approve checks @kieran-ryan

@jacob-wieland-gematik
Copy link
Contributor Author

I am not able to generate Parser.php locally, so not sure what to do here @kieran-ryan

@mpkorstanje
Copy link
Contributor

@jacob-wieland-gematik are you able to generate the php parser using the docker container?

https://github.com/cucumber/gherkin/blob/main/CONTRIBUTING.md#generating-parsers

@jacob-wieland-gematik
Copy link
Contributor Author

@jacob-wieland-gematik are you able to generate the php parser using the docker container?

https://github.com/cucumber/gherkin/blob/main/CONTRIBUTING.md#generating-parsers

No, there seems to be a problem doing that under windows (probably CRLF vs LF related) that hinders this generation.

When change the CRLF of the generated classes.php to LF and then do the generation steps manually, I was able to generate Parser.php

@mpkorstanje

@jacob-wieland-gematik
Copy link
Contributor Author

When will this be merged? @mpkorstanje @kieran-ryan

@mpkorstanje mpkorstanje changed the title comments should not change parser state Comments should not change parser state Jan 12, 2025
@mpkorstanje mpkorstanje changed the title Comments should not change parser state Allow comment inside feature description Jan 12, 2025
@mpkorstanje mpkorstanje self-assigned this Jan 12, 2025
@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jan 12, 2025

Always a bit hard to say, depends on availability. Same for releases. 😄

But hopefully bit more regularly now the holidays are over.

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Looks good overall. I am missing a test with a comment in the middle of a description.

Do you have time to add a scenario to descirptions.feature?

@mpkorstanje mpkorstanje changed the title Allow comment inside feature description Allow comment inside descriptions Jan 12, 2025
@jacob-wieland-gematik
Copy link
Contributor Author

Looks good overall. I am missing a test with a comment in the middle of a description.

Do you have time to add a scenario to descirptions.feature?

I can try.

@jacob-wieland-gematik
Copy link
Contributor Author

Added tests @mpkorstanje

@jacob-wieland-gematik
Copy link
Contributor Author

Added missing compare files. @mpkorstanje

@jacob-wieland-gematik
Copy link
Contributor Author

Seems all is well now. Any chance we can merge this? @mpkorstanje @kieran-ryan

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Cheers! Looks good now. Much appreciated.

@mpkorstanje mpkorstanje merged commit 93339df into cucumber:main Jan 23, 2025
34 checks passed
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.

Comment inside Free Form Description not allowed
3 participants