Add introspection to eckit::Configuration

[wdeconinck]

This proposed addition allows to make sure that a configured value is of an expected type without resorting to try / catch.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 73.10924% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 61.97%. Comparing base (4fdc02f) to head (44e7ddd).

Files	Patch %	Lines
src/eckit/config/Configuration.cc	56.16%	32 Missing :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #101      +/-   ##
===========================================
+ Coverage    61.94%   61.97%   +0.03%     
===========================================
  Files          899      900       +1     
  Lines        49652    49799     +147     
  Branches      3730     3745      +15     
===========================================
+ Hits         30756    30865     +109     
- Misses       18896    18934      +38     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[pmaciel]

Sorry to have taken some time to return to this.

So, eckit::Parametrisation is a eckit::Configuration's base class, with just the barebones interface full of pure virtual methods (like a Java interface). Configuration holds a "root" eckit::Value supporting all the nested structrure of values we care to contain.

Adding introspection to the Configuration is certainly useful -- but, with the reflection time I had to think about the class structure, I think the current version has a few inconsistencies (obviously, a matter of opinion):

1. the list of new "isType..." is not complete in respect to the internal eckit::Value possible related types (via eckit::Content); I understand it is following the <type_traits> conterparts, but it isn't comprehensive in that respect either. Hence I would suggest to convert this family of methods to a template "is<Type>()", or overloading "has<Type>()" maybe, where one can pass the type_traits definition, or otherwise map to the eckit::Value possible derived types?
2. If the class contains a "root" eckit::Value, why not expose it like in const eckit::Value& getValue(const std::string& name), which we could expose the introspection of eckit::Value instead?
3. The same logic would be applicable to the isConvertible -- because it would better semantically apply (directly) to eckit::Value than (indirectly) to eckit::Configuration.

But in any case I'm even a proponent to change/revamp eckit::Value completely, as modern (and not so modern) C++ has both introspection (<type_traits>) and conversion (std::to_string, std::istream, leveraging std::is_arithmetic and the like) native support. There's a whole variety of unrelated eckit::Value related types via eckit::Content like "Date", "DateTime", and "Time" that wouldn maybe better be moved downstream so this hierarchy could be closer to native C++; Some of these related types also offer limited operations like compare..., add..., sub..., mul..., div..., mod... which would likely be better suited downstream. This PR isn't touching this (neither are immediate dependant projects like Atlas or MIR, but of course configuration is suitable for many other packages), and a revision would allow for a significant simplification and adhering to the standards more closely.

So I don't know if this should follow a cleanup. As a compromise, notwithstanding a complete review of eckit::Configuration and eckit::Value, I suggest adding a Configuration::getValue accessor and moving the introspection/conversion to the eckit::Value class?

So consider my tacit aproval, as of course I know this PR adds significant convenience. I'm just not sure it is in the right place, considering upcoming use of these classes.

[pmaciel]

(A possible Configuration::getValue could be just a redirect of the Configuration::lookUp method, which is currently protected).

[wdeconinck]

The design of Configuration was that eckit::Value is an implementation detail. see e.g. comment

class Configuration : public Parametrisation {
    /// @note Do NOT expose eckit::Value in the interface of configuration
    ///       eckit::Value should remain an internal detail of configuration objects
    ///       Clients should use typed configuration parameters

I don't mind but others may have issue with "const Value& Configuration::getValue( name );"

[tlmquintino]

The design of Configuration was that eckit::Value is an implementation detail. see e.g. comment

class Configuration : public Parametrisation {
    /// @note Do NOT expose eckit::Value in the interface of configuration
    ///       eckit::Value should remain an internal detail of configuration objects
    ///       Clients should use typed configuration parameters

I don't mind but others may have issue with "const Value& Configuration::getValue( name );"

that's what I meant with my comment on the other thread

[wdeconinck]

@tlmquintino , @simondsmart could you please advise on the desired API as discussed above?

[pgeier]

The design of Configuration was that eckit::Value is an implementation detail. see e.g. comment

class Configuration : public Parametrisation {
    /// @note Do NOT expose eckit::Value in the interface of configuration
    ///       eckit::Value should remain an internal detail of configuration objects
    ///       Clients should use typed configuration parameters

I don't mind but others may have issue with "const Value& Configuration::getValue( name );"

But in multio we have exactly the problem that the assumption of having typed configuration parameters is too strong. We have actions that may contain a subconfigurations with configurations and settings for other libraries like eccodes & MIR to be passed down.

Without that kind of feature, we would have to add a lot of more that is just explicitly performing a lot of explicity queries to the configuration. This list might grow very large and potentially change with updates to these libraries.

[wdeconinck]

@tlmquintino , @simondsmart could you please advise on the desired API as discussed above?

ping @tlmquintino @simondsmart

[wdeconinck]

I think 4 months should be more than enough for a PR waiting for review. Unless someone objects before Friday 24/05/2024, I will assume everything is OK.

[pgeier]

We will need this to in multio to resolve all of the deprecation warnings.

Moreover I will need some of it in my mars2grib work on metkit to distinguish between int, string and float.