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

Feature/cpp #152

Merged
merged 27 commits into from Jun 1, 2023
Merged

Feature/cpp #152

merged 27 commits into from Jun 1, 2023

Conversation

chybz
Copy link
Contributor

@chybz chybz commented May 3, 2023

🤔 What's changed?

  • Added C++ message generation support
  • Added CMake library

⚡️ What's your motivation?

While adding C++ support in Gherkin, saw that other implementations were using this repository, so decided to contribute in here as well.

🏷️ What kind of change is this?

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

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • 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.

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.

No external dependencies, so no JSON support as of now, hence no tests.

I think we can do without tests for serialization, but I would expect at the very least a test-cpp.yml in the github workflows to ensure the project compiles.

Arbitrary dependency on C++20

I'm not familiar with CPP. Where does the dependency on C++20 come in to play?

cpp/src/lib/cucumber-messages/CMakeLists.txt Outdated Show resolved Hide resolved
@jenisys
Copy link

jenisys commented May 10, 2023

Arbitrary dependency on C++20

You should not require the C++20 standard in the CMake file,
if your source code does not require some of the features from the C++20 standard.

POSSIBLE APPROACHES:

  • Define CMAKE_CXX_STANDARD as default if no C++ standard is defined.
    You should support that the user overrides it via cmake -DCMAKE_CXX_STANDARD=17 ... on the command line.
    In addition, this approach allows that a parent CMakeLists.txt file (that wants to use this library as part of the overall build process) predefines the CMAKE_CXX_STANDARD.
  • BETTER APPROACH: Your library(s)/program(s) should specify which C++ features are required.
    This determines which C++ standard is used (or it fails if the C++ compiler does not support these standard(s)/features)
  • The C++ compiler version (as part of the toolchain) defines the default C++ standard that is used by default
# -- FILE: CMakeLists.txt
# EXAMPLE FOR SOLUTION 1
if(NOT DEFINED CMAKE_CXX_STANDARD)
    set(CMAKE_CXX_STANDARD 20)  # Use C++20 standard as default
endif()

@chybz
Copy link
Contributor Author

chybz commented May 10, 2023

Yes, definitely. That was just me being lazy. Thank you very much !

@chybz
Copy link
Contributor Author

chybz commented May 31, 2023

@mpkorstanje, @jenisys: repository should be in a decent state now, could you please comment on what I should do next ? (A bit more work is necessary for CI/CD). Thank you

@chybz chybz merged commit 17f4fc4 into cucumber:main Jun 1, 2023
26 checks passed
@chybz chybz deleted the feature/cpp branch June 1, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants