Skip to content

Conversation

aligirayhanozbay
Copy link

This PR adds traits for the reflect-cpp json library

@aligirayhanozbay aligirayhanozbay changed the title Reflect-cpp compatibility Reflect-cpp traits Aug 12, 2025
@Thalhammer
Copy link
Owner

Hi and thanks for extending jwt-cpp.
Would it be possible to add some CI testing to ensure we don't break any of the code in the future ?
In addition the CMake scripts need modification to ensure the new files are correctly installed along the other headers.

@aligirayhanozbay
Copy link
Author

@Thalhammer Added tests, using FetchContent if reflectcpp is not found

@aligirayhanozbay
Copy link
Author

@Thalhammer I've added everything needed for CI, including

  • Installation of reflect-cpp
  • Building the example executable for the reflect-cpp traits
  • Building the tests for the reflect-cpp traits
  • Running the tests for the reflect-cpp traits

On my fork, the CI runs successfully for everything assosciated with the new reflect-cpp trait. Unfortunately, one CI item seems to fail (JWT CI / coverage (pull_request)) but I see that is broken on main in the repo and that you're in the process of fixing it.

Please let me know if you'd like any further modifications.

@prince-chrismc
Copy link
Collaborator

I noticed the lint ci is missing the new json library, i am okay with it missing.

You might want to update the readme to share your excellent work!

@aligirayhanozbay
Copy link
Author

aligirayhanozbay commented Aug 26, 2025

@Thalhammer @prince-chrismc

Per your comments, I have

  • Removed the use of Ninja
  • Removed the set(CMAKE_EXPORT_COMPILE_COMMANDS ...) line
  • Modified .github/workflows/cmake.yml to re-use the action from .github/actions/install/reflect-cpp/action.yml

Could you clarify what you mean by updating the readme? I don't see anything about any of the other traits or contributors in the README.md file. Do you mean adding badges to docs/traits.md?

@Thalhammer
Copy link
Owner

Do you mean adding badges to docs/traits.md?

Yes that section used to be in the readme itself before it got too large 😅

@aligirayhanozbay
Copy link
Author

aligirayhanozbay commented Aug 28, 2025

@Thalhammer @prince-chrismc

Added reflect-cpp to docs/traits.md. Any outstanding items before we can merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants