Skip to content

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Sep 11, 2024

This PR adds the yaml-cpp to support yaml format for file config which gives the alias feature to reuse config.
welcome any suggestion of yaml library.

@yhmtsai yhmtsai self-assigned this Sep 11, 2024
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. mod:core This is related to the core module. labels Sep 11, 2024
@MarcelKoch
Copy link
Member

Regarding the library choice, yaml-cpp seems to be the most popular, but rapidyaml seems to be a good alternative. rapidyaml supports resolving anchors automatically, which would simplify our parse implementation (see here).

@MarcelKoch MarcelKoch self-requested a review February 13, 2025 11:50
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

LGTM, only few minor points

Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

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

LGTM!

@upsj
Copy link
Member

upsj commented Feb 19, 2025

I wish there was a better library for YAML parsing, but I guess this will have to do. There are some issues with how yaml-cpp handles assignment and copies jbeder/yaml-cpp#1275. And to quote the developer:

Unfortunately, I don't have time to work on this library any more

Not blocking, just something to keep in mind in case we find a better library out there.

@yhmtsai
Copy link
Member Author

yhmtsai commented Feb 19, 2025

Yes, I also faced the issue that it still accepts the invalid yaml file.
like

arr: &arr1 [1, 2, 3]
base:
  <<: *arr1

which should be invalid because <<: can only used from map not array.
it accepts this file unfortunately.
As @MarcelKoch mentioned, rapidyaml might be another good library to use.
yaml-cpp still served larger group, so I will still merge this first.
Because config is not tied to any specific parse, we can create rapidyaml extension for config later

@yhmtsai yhmtsai merged commit 14a761e into develop Feb 19, 2025
10 of 11 checks passed
@yhmtsai yhmtsai deleted the yaml-config branch February 19, 2025 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1:ST:ready-to-merge This PR is ready to merge. 1:ST:skip-full-test mod:core This is related to the core module. reg:build This is related to the build system. reg:testing This is related to testing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants