Feature/introspection #63
Conversation
|
nathanhhughes
left a comment
nathanhhughes
left a comment
There was a problem hiding this comment.
Looks great! Very excited to try it out
config_utilities/src/commandline.cpp
Outdated
| if (!fs::exists(file)) { | ||
| std::stringstream ss; | ||
| ss << "File " << file << " does not exist!"; | ||
| ss << "File '" << file << "' does not exist!"; |
There was a problem hiding this comment.
(minor) the streaming operator for filepaths adds quotes by default (I think you end up with the literal '"/some/filepath"' unless you do file.string()
config_utilities/src/commandline.cpp
Outdated
| } catch (const std::exception& e) { | ||
| std::stringstream ss; | ||
| ss << "Failure for " << file << ": " << e.what(); | ||
| ss << "Failure for '" << file << "': " << e.what(); |
There was a problem hiding this comment.
ditto for the filesystem streaming operator
config_utilities/CMakeLists.txt
Outdated
| RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}) | ||
| install(DIRECTORY introspection_viewer/ DESTINATION ${CMAKE_INSTALL_BINDIR}) | ||
| install(PROGRAMS introspection_viewer/introspection_viewer.py | ||
| RENAME config_utilities_viewer |
There was a problem hiding this comment.
minor preference for config-utilities-viewer to match composite-configs stylistically
| ~RegistrationGuard() { | ||
| internal::ConfigFactory<BaseT>::template removeEntry<ConfigT>(type); | ||
| internal::ObjectWithConfigFactory<BaseT, Args...>::removeEntry(type); | ||
| internal::ModuleRegistry::registerConfig<DerivedT, ConfigT>(type); |
There was a problem hiding this comment.
Am I missing something for why this has to be re-registered in the destructor? I don't think this changes too much when running the unit tests via ctest, but might break the factory registration tests when running all the tests locally
There was a problem hiding this comment.
Good catch! I have honestly no idea how that got there 😱
Remaining todos:
Otherwise runs, feel free to give it a try and LMK what you think!