Skip to content

Conversation

@clrudolphi
Copy link
Contributor

🤔 What's changed?

The description comments in the schema for a TestStep's StepDefinitionIds was mistakenly placed on the neighboring StepMatchArgumentsList field. This PR moves that text to the description field of the StepDefinitionIds field.

⚡️ What's your motivation?

Avoid confusion when reading the code.

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ Anything particular you want feedback on?

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • [X ] I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

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

A description of the TestStep's StepDefinitionIds was mistakenly placed as comment on the StepMatchArgumentsList. This commit fixes that by moving the comment up to the StepDefinitionIds field.
@mpkorstanje
Copy link
Contributor

@clrudolphi build is failing. You should run the code generator for all languages.

@clrudolphi
Copy link
Contributor Author

You should run the code generator for all languages.

Is the premise of the PR correct (that the comment was attributed to the wrong property)?
If so, I'll figure out how to regenerate all.

@clrudolphi
Copy link
Contributor Author

build is failing

Pardon my ignorance, but it appears to me from looking at the log that the build did regenerate for all languages. The step of the build that is reporting the failure seems to be doing a diff against a prior run and finding the differences in the comments and flagging that as a failure. Do certain languages maintain a 'golden' copy of generated code that all subsequent builds must match? How would I go about updating that?

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Sep 19, 2024

No problems. Have a look at CONTRIBUTING.md. For most projects that is where we keep the "how to" information (and if we not, feel free to reorganize a bit).

As with Gherkin, for messages too we want to limit the dependencies involved in code generation. So for example building Python should not depend on ruby. And generating the code should not depend on python. To solve that problem all generated code is checked in. Then to ensure the code is generated consistently, CI reruns the code generation and checks for differences.

@mpkorstanje
Copy link
Contributor

Is the premise of the PR correct (that the comment was attributed to the wrong property)?

Yes. Looks like it is correct to me.

@clrudolphi
Copy link
Contributor Author

No problems. Have a look at CONTRIBUTING.md. For most projects that is where we keep the "how to" information (and if we not, feel free to reorganize a bit).

As with Gherkin, for messages too we want to limit the dependencies involved in code generation. So for example building Python should not depend on ruby. And generating the code should not depend on python. To solve that problem all generated code is checked in. Then to ensure the code is generated consistently, CI reruns the code generation and checks for differences.

Running make clean-all generate-all on my machine fails. It seems one needs Go installed for the generation to work for the Go lang.

vscode ➜ /workspaces/messages (Dotnet_AddDefensiveDeserializationLogic) $ make clean-all generate-all
rm -f messages.md
cd cpp && make clean
make[1]: Entering directory '/workspaces/messages/cpp'
rm -rf build .configured
make[1]: Leaving directory '/workspaces/messages/cpp'
cd go && make clean
make[1]: Entering directory '/workspaces/messages/go'
rm -f messages.go
make[1]: Leaving directory '/workspaces/messages/go'
cd java && make clean
make[1]: Entering directory '/workspaces/messages/java'
rm -rf src/generated/java/io/cucumber/messages/types
make[1]: Leaving directory '/workspaces/messages/java'
cd javascript && make clean
make[1]: Entering directory '/workspaces/messages/javascript'
rm -f src/messages.ts
make[1]: Leaving directory '/workspaces/messages/javascript'
cd perl && make clean
make[1]: Entering directory '/workspaces/messages/perl'
rm -f lib/Cucumber/Messages.pm
make[1]: Leaving directory '/workspaces/messages/perl'
cd php && make clean
make[1]: Entering directory '/workspaces/messages/php'
rm -rf build/messages.php
rm -rf build/Generated*.php.tmp
rm -rf src-generated/*
make[1]: Leaving directory '/workspaces/messages/php'
cd ruby && make clean
make[1]: Entering directory '/workspaces/messages/ruby'
find ./lib/cucumber/messages/*.rb ! -name 'message.rb' -type f -exec rm -f {} +
make[1]: Leaving directory '/workspaces/messages/ruby'
cd dotnet && make clean
make[1]: Entering directory '/workspaces/messages/dotnet'
rm -rf Cucumber.Messages/generated/*.cs
make[1]: Leaving directory '/workspaces/messages/dotnet'
cd cpp && make generate
make[1]: Entering directory '/workspaces/messages/cpp'
ruby ../codegen/codegen.rb Generator::Cpp cpp.hpp.erb > Generated.hpp.tmp
ruby ../codegen/codegen.rb Generator::Cpp cpp.enum.hpp.erb >> Generated.hpp.tmp
csplit --quiet --prefix=Generated --suffix-format=%02d.hpp.tmp --elide-empty-files Generated.hpp.tmp /^[A-Za-z_.]*[.][ch]pp/ {*}
rm Generated.hpp.tmp
for file in Generated**; do \
    F=$(head -n 1 $file | tr -d '\r\n'); \
    if [ -n "$F" ]; then \
        tail -n +2 $file > include/messages/cucumber/messages/$F; \
    fi; \
    rm $file; \
done
mv include/messages/cucumber/messages/*.cpp src/lib/messages/cucumber/messages/ || true
make[1]: Leaving directory '/workspaces/messages/cpp'
cd go && make generate
make[1]: Entering directory '/workspaces/messages/go'
ERROR: go is required.
make[1]: *** [Makefile:12: require] Error 1
make[1]: Leaving directory '/workspaces/messages/go'
make: *** [Makefile:61: generate-go] Error 2

I don't have the time or expertise to have all of these language SDKs installed.
Can someone else take this PR forward?

@mpkorstanje
Copy link
Contributor

I thought I removed the dependency on go.

fb871f4

I'll check tomorrow.

@clrudolphi
Copy link
Contributor Author

I thought I removed the dependency on go.

fb871f4

I'll check tomorrow.

The go Makefile has this section still in it:

require: ## Check requirements for the code generation (ruby and go are required)
	@ruby --version >/dev/null 2>&1 || (echo "ERROR: ruby is required."; exit 1)
	@go version >/dev/null 2>&1 || (echo "ERROR: go is required."; exit 1)

@mpkorstanje
Copy link
Contributor

Ah yeah. That will do it.

@mpkorstanje mpkorstanje merged commit 026ddc9 into cucumber:main Sep 20, 2024
@mpkorstanje
Copy link
Contributor

Cheers! Thanks for spotting this!

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.

2 participants