Rewrite ConfigManager (second attempt)
New proposal replacing !111 (closed). This currently mostly implement the same functionality as in the first attempt (including the little bit unrelated added tests) and allows for more flexiblity, see initial discussion here.
The parsing of CLI arguments is now done by a separate class,
OptionParser
which makes it a bit more obvious that these aren't directly applied to the configs but have to be manually added later by callingapplyOptions()
. Having this in theConfigManager
always confused me.Furthermore, this implements changes which fix #112 (closed) and add the version number as well as all global configuration parameters to the data files written by the
ROOTObjectWriter
.
In this MR the ConfigManager
stores the global configuration and the configuration of all modules. This allows the framework and individual modules to change and access centrally stored configuration objects. Most importantly this gives output writers the opportunity to output the final configuration with default keys and the like.
Some points for discussion
- The
OptionParser
is now almost completely handled in theConfigManager
again. ThegetOptionParser
is only used to provide parsing options. I think we should probably remove thegetOptionParser
and move the option parsing toloadOptions
(we can keep the class itself). - This changes the API to move the ownership of the modules configuration from the modules themselves to the config manager. This is technically not so nice from a modularity perspective, but I think this is only a minor conceptual thing and the extra flexibility of this approach outweighs that.
- It is probably good to also add a
getDetectorConfigurations
and move the reading and parsing of the detector file from theGeometryManager
to theConfigManager
. More questionable if we should integrate the configurations for the detector models to the config manager as well (I think for now we shouldn't). - As in the previous approach I use the empty section now as a name for the global configuration and also changed the exceptions to comply to that. An unnamed configuration is not very nice, but I think this approach still makes more sense.
- The configuration objects are stored with their original name. This is done to keep in line with the previous implementation and ensures that the name as it appears in the configuration file is printed in case of errors. This means however that the unique name cannot be accessed directly and therefore I added a required key 'unique_name' to every instance configuration to be used for this. This is not a very elegant approach though... any other ideas?