kamchatka-volcano / figcone

Read JSON, YAML, TOML, XML or INI configuration by declaring a struct
Microsoft Public License
100 stars 2 forks source link

Unregistered data handler (fixes #16) #17

Closed DrDub closed 7 months ago

DrDub commented 9 months ago

This is a first attempt at #16 using a std::function and a global (fixed) default.

Includes two new test cases.

My feeling is that it might be too simple given the rest of the design but it is user facing so it might be good keeping it simpler.

Potential issues:

I'll be happy to evolve the changes and address some of these issues based on your review.

Once the changes are good, I can include also changes to the README and maybe an example if you think that'll be good.

kamchatka-volcano commented 9 months ago

Hi, thanks a lot for your contribution! Currently, it doesn't cover this requirement: "It should be possible to register a handler for a specific Config type." Therefore, I think we should work on this more, as I want to support the following use case.

Imagine if we have a configuration like this:

{
    "header" : {"id" : 20, "random_field" : "foo", "random_field_2" : "bar"},    
    "data" : ["Hello", "world"]} 
}

The "header" node must contain the "id" field, but it can also have random fields that aren't important for our app. So, if we register it with figcone:

struct HeaderCfg : figcone::Config{
    FIGCONE_PARAM(id, std::string);    
};

struct Cfg : figcone::Config
{
    FIGCONE_NODE(header, HeaderCfg);
    FIGCONE_PARAM(data, std::vector<std::string>);    
};

The problem is that, with your current solution, we can't allow only the "header" subnode to have random fields(

What I had in mind for this issue is using template specializations to be able to register handlers for the necessary config types used in subnodes. Instead of:

static inline UnknownDataHandler defaultUnknownDataHandler()
{
    return [](std::string nodeName, std::string paramName, StreamPosition position) {
        if(!nodeName.empty())
            throw ConfigError{"Unknown node '" + nodeName + "'", position};
        throw ConfigError{"Unknown param '" + paramName + "'", position};
    };
}

We place the regular exception-throwing handler in the template:

template<typename TConfig>
struct UnknownDataHandler{
    void operator()(std::string nodeName, std::string paramName, StreamPosition position)
    {
        if(!nodeName.empty())
            throw ConfigError{"Unknown node '" + nodeName + "'", position};
        throw ConfigError{"Unknown param '" + paramName + "'", position};
    }
};

And instead of calls like this:

 UnknownDataHandler handler = handler_ ? handler_ : defaultUnknownDataHandler();
 handler(nodeName, std::string(), position);

We can use it like this:

UnknownDataHandler<TConfig>{}(nodeName, std::string(), position);

Then the user could add the UnknownDataHandler specializations for only those node types that require a handler for unregistered fields:

template<>
struct UnknownDataHandler<HeaderCfg>{ 
    //Do nothing and allow HeaderCfg to have any fields besides the registered ones.
    void operator()(std::string nodeName, std::string paramName, StreamPosition position)
    {        
    }
};

Check out how cmdlime uses this technique to register post processors (https://github.com/kamchatka-volcano/cmdlime#using-post-processors)

Right now, the big problem is that UnknownDataHandler should be called inside the ConfigReader::load() method, which doesn't have access to the current config type. It's also not possible to convert it to the template method taking a config type as a template argument because load() is a virtual method that is invoked polymorphically through the detail::IConfigReader* pointers. So currently, this problem needs some research; right now, I can't say if it's even possible with the current design.

kamchatka-volcano commented 7 months ago

I'm closing the PR, as this functionality was implemented in v3.0.0