Add config parser utilities #175

tweska · 2025-10-09T10:13:02Z

Similar to what we have in datamod::core for metadata, but now simplified implementation to parse action configurations.

This is not meant to be the final version yet, but rather a simple prototype we can discuss.

🌈🌦️📖🚧 Documentation 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/multio/pull-requests/PR-175

codecov-commenter · 2025-10-09T10:39:20Z

Codecov Report

❌ Patch coverage is 84.81013% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.41%. Comparing base (c7c8de1) to head (9378ead).

Files with missing lines	Patch %	Lines
src/multio/util/ConfigParser.cc	30.61%	34 Missing ⚠️
src/multio/util/ConfigParser.h	95.23%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #175      +/-   ##
===========================================
- Coverage    56.60%   56.41%   -0.19%     
===========================================
  Files          316      315       -1     
  Lines        20490    20473      -17     
  Branches      1569     1594      +25     
===========================================
- Hits         11598    11550      -48     
- Misses        8892     8923      +31

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dsarmany

This is much-much nicer. Thank you.

src/multio/util/ConfigParser.cc

src/multio/util/ConfigParser.h

Co-authored-by: Philipp Geier <[email protected]>

Field -> Entry config -> localConfig when type is eckit::LocalConfiguration

tweska · 2025-10-13T15:51:02Z

src/multio/util/config/detail/Entry.h

+template <typename TEnum, std::enable_if_t<std::is_enum_v<TEnum>, bool> = true>
+struct EnumTrait;


Maybe we should move the EnumTrait somewhere else in multio::util and implement toString and fromString methods.

This would require a discussion and a separate PR.
With this PR we intended to create something small and specifically for configs.

pgeier

Made some comments and have some questions.

Is there a reason for renaming it back to Entry?

There are a couple of TODO: Improve error message!. Are these fixed or still need adjustment?

Can we create a test that prints a error message? Are they user friendly and carry enough context?

pgeier · 2025-10-14T06:37:44Z

src/multio/util/config/detail/Entry.h

+template <typename TEnum, std::enable_if_t<std::is_enum_v<TEnum>, bool> = true>
+struct EnumTrait;


This would require a discussion and a separate PR.
With this PR we intended to create something small and specifically for configs.

pgeier · 2025-10-14T06:41:47Z

src/multio/util/config/Parser.h

+namespace multio::util::config {
+
+
+/**


I'm not sure which code style we adopt to.

In Metkit/FDB we use /// and @COMMAND. But in the multio header we do /** */ and \command.
To be consistent with the other repositories, I opt for /// & @COMMAND.

Ah, I successfully managed to mix both styles somehow. :^)

Well, it is quite a mix in all repositories, including fdb/metkit. There is a clear preference there for /// & @, but examples for both. In, e.g. atlas and mir, we have a more 'even' mix. So I wouldn't lose too much sleep over it.

But in general, trying to converge on a single style would be preferable, of course.

pgeier · 2025-10-14T06:42:55Z

src/multio/util/config/Parser.h

+ * @throws If an unknown key is present in the localConfig, or if a value cannot be parsed
+ */
+template <typename TConfig>
+TConfig parseConfig(const eckit::LocalConfiguration& localConfig) {


Given that we separate details, this body should also be moved to details.

Can you be more precise, where should this be moved?

For example:

template <typename TConfig> TConfig parseConfig(const eckit::LocalConfiguration& localConfig) { return details::parseConfig<TConfig>(localConfig); }

pgeier · 2025-10-14T06:43:56Z

tests/multio/test_multio_config_parser.cc

+
+
+// Double value
+// TODO(knobel) parse double


Remove comments.

Only when we implement the test.

tweska · 2025-10-14T07:08:39Z

Made some comments and have some questions.

Is there a reason for renaming it back to Entry?

There are a couple of TODO: Improve error message!. Are these fixed or still need adjustment?

Can we create a test that prints a error message? Are they user friendly and carry enough context?

All the TODO's are still not addressed. I focused on the structure for now.

dsarmany · 2025-10-14T08:04:38Z

tests/multio/test_multio_config_parser.cc

+
+template <>
+struct multio::util::config::detail::EnumTrait<multio::test::FancyEnum> {
+    static constexpr std::array values{


Why is this an array of std::pairs, rather than an std::map (which is also a container of std::pairs)?

The intention is the make the definition of the string mapping readable in the header file.
With an array<pair>> we can simply create a static constexpr member. That would not be possible with a map/unordered_map - instead we would either

create a static const - which has downside that the definition/initialization must be put in a compilation unit (.cc file)

or create a static function that returns a map and itself uses a static const variable - would be just a few lines more of code e.g.
template <> struct multio::util::config::detail::EnumTrait<multio::test::FancyEnum> { static const auto& values() { static const map<string, Enum> values_{ std::pair{multio::test::FancyEnum::Rock, "rock"}, std::pair{multio::test::FancyEnum::Paper, "paper"}, std::pair{multio::test::FancyEnum::Scissors, "scissors"}}; return values_; } };

dsarmany · 2025-10-14T08:14:45Z

tests/multio/test_multio_config_parser.cc

+
+// Configuration to struct parsing
+
+struct MyConfig {


So if you think about the configuration as key-value pairs, then 'required' and 'optional' are about the existence of the keys in the config. And we use the enum trait to restrict the allowable values of any given key (when this restriction makes sense). Is my understanding correct?

Yes, if by config you mean the eckit::LocalConfiguration. In the example below the idea is:

name is required, because there is no default, and it is not an std::optional;

enabled is optional, even though it is not an std::optional, but there is a default value;

choice is optional and can be unset after parsing, because it is an std::optional.

The enum makes sense for situations where there are a finite number of possible values. For example for statistics operations (although that would be an std::vector<SomeEnumForTheOperation>).

tweska force-pushed the feature/config-parser branch from c12921f to cad1163 Compare October 9, 2025 11:10

dsarmany requested changes Oct 9, 2025

View reviewed changes

src/multio/util/ConfigParser.cc Outdated Show resolved Hide resolved

src/multio/util/ConfigParser.h Outdated Show resolved Hide resolved

Add config parser utilities

b70feb5

Co-authored-by: Philipp Geier <[email protected]>

tweska force-pushed the feature/config-parser branch from cad1163 to b70feb5 Compare October 13, 2025 08:02

tweska added 2 commits October 13, 2025 13:02

Rename methods and variables

9378ead

Field -> Entry config -> localConfig when type is eckit::LocalConfiguration

Update config parser directory and namespace structure

f7415a6

tweska commented Oct 13, 2025

View reviewed changes

tweska requested review from MircoValentiniECMWF, dsarmany, pgeier and stefaniereuter October 13, 2025 15:51

pgeier requested changes Oct 14, 2025

View reviewed changes

dsarmany reviewed Oct 14, 2025

View reviewed changes

		template <typename TEnum, std::enable_if_t<std::is_enum_v<TEnum>, bool> = true>
		struct EnumTrait;

Uh oh!

Add config parser utilities #175

Are you sure you want to change the base?

Add config parser utilities #175

Conversation

tweska commented Oct 9, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dsarmany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgeier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgeier Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tweska commented Oct 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgeier Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsarmany Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tweska commented Oct 9, 2025 •

edited by github-actions bot

Loading

codecov-commenter commented Oct 9, 2025 •

edited

Loading

pgeier Oct 14, 2025 •

edited

Loading

pgeier Oct 14, 2025 •

edited

Loading

dsarmany Oct 14, 2025 •

edited

Loading