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

php: Parser is not generated with the right formatting #35

Closed
Tracked by #2
mpkorstanje opened this issue Nov 8, 2022 · 3 comments · Fixed by #46
Closed
Tracked by #2

php: Parser is not generated with the right formatting #35

mpkorstanje opened this issue Nov 8, 2022 · 3 comments · Fixed by #46

Comments

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Nov 8, 2022

🤔 What's the problem you've observed?

One of the goals of setting up the workflow for this project was that generating parsers should only depend on Berp. This simplifies the image needed to build this project. However PHP also formatted the parser using vendored tools. This effectively adds a dependency on PHP.

gherkin/php/Makefile

Lines 47 to 52 in 6b84ba8

build/classes.php: gherkin-php.razor gherkin.berp
$(berp-generate-parser)
csplit --quiet --prefix=build/Generated --suffix-format=%02d.php.tmp --elide-empty-files build/classes.php /^.*.php$$/ {*}
for file in build/Generated**; do mkdir -p src-generated/$$(head -n 1 $$file | sed 's/[^/]*.php$$//'); done
for file in build/Generated**; do tail -n +2 $$file > src-generated/$$(head -n 1 $$file); rm $$file; done
vendor/bin/php-cs-fixer --diff fix src-generated

✨ Do you have a proposal for making it better?

Either

  • fix gherkin-php.razor to produce correctly formatted files
  • or ignore the problem. It is generated code.

💻 Additional context

In #3 I've removed the code formatter and fixed most of the razor file. I couldn't fix everything.

@mpkorstanje
Copy link
Contributor Author

@ciaranmcnulty what do you think?

@mpkorstanje mpkorstanje mentioned this issue Nov 8, 2022
71 tasks
@ciaranmcnulty
Copy link
Contributor

Yeah I agree this should be changed. Probably this was just laziness understanding the berp templates on my part.

It makes sense to me, now that we're unpicking the build steps, to have the codegen here rely only on Berp, but validate the formatting as part of the PHP test run by using php-cs-fixer with appropriate flags to not 'fix' stuff.

I made a similar change in the Messages buildkit spike thingy

IIRC the Golang build does go fmt during code generation, so maybe we should change it there too.

@mpkorstanje
Copy link
Contributor Author

IIRC the Golang build does go fmt during code generation, so maybe we should change it there too.

Already fixed that.

It makes sense to me, now that we're unpicking the build steps, to have the codegen here rely only on Berp, but validate the formatting as part of the PHP test run by using php-cs-fixer with appropriate flags to not 'fix' stuff.

Makes sense.

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 a pull request may close this issue.

2 participants