Feature/external plugins #23
Conversation
| * @brief Populate factory methods from a provided shared object library | ||
| * @param library_path Relative or absolute path to library (without extension or lib prefix) | ||
| * | ||
| * Warning: loading modules or code from external libraries that are not linked into the current |
| if (map.find(type) != map.end()) { | ||
| if (!type_info.empty()) { | ||
| Logger::logError("Cannot register already existent type '" + type + "' for " + type_info + "."); | ||
| class ModuleRegistry { |
There was a problem hiding this comment.
Minor: It might help to add brief docstrings to the different classes (what they are supposed to do / what's the difference to other ones) for easier reading into the code (there's a few parts that sound somewhat similar on a quick pass)
| using Factory = FactoryMethod<BaseT, Args...>; | ||
| if (locked()) { | ||
| if (hasModule(key, type)) { | ||
| return false; |
There was a problem hiding this comment.
Does it make sense to log a warning here or does this intentionally fail quietly?
There was a problem hiding this comment.
Intentionally silently fails (I'll add a note). The check is to avoid compiled-in factories that get re-registered in the external library (e.g., StdoutLogger)
| static FactoryMethod<BaseT, Args...> getModule(const std::string& type, | ||
| const std::string& registration_info, | ||
| bool skip_first_arg = false) { | ||
| using Factory = FactoryMethod<BaseT, Args...>; |
There was a problem hiding this comment.
Nitpick: factories and factory methods are technically not the same thing (and typically similar but not identical patterns). I don't think people will be too confused here though.
| } | ||
|
|
||
| void ModuleRegistry::removeModule(const ModuleInfo& key, const std::string& type) { | ||
| { // module scope |
There was a problem hiding this comment.
Is there a reason for these being scoped?
There was a problem hiding this comment.
I didn't feel like coming up with specific variable names for iter so the scope is to be able to use it twice
| config::internal::Formatter(): { | ||
| 'asl' (config::internal::AslFormatter), | ||
| }, | ||
| config::internal::Logger(): { | ||
| 'stdout' (config::internal::StdoutLogger), | ||
| 'test_logger' (config::test::TestLogger), | ||
| }, | ||
| config::test::Base(_, int): { |
There was a problem hiding this comment.
If I understand the _ are skip-first arg registrations? It's not very clear to me how to read these (the initial idea of the design was to show the user how to invoke all these types/factories. Not clear to me how I would go about this for a Object(_)? Open to changes though 🙂
There was a problem hiding this comment.
Yeah, sorry meant to check with you about what you wanted here and then forgot. The _ is to mark that these are actually ObjectWithConfig registrations. The reason for the skip first is because otherwise you get YAML::Node as the first arg which looked ugly and was confusing (though now that I think about it, it think ObjectFactory::create would dispatch properly to the ObjectWithConfigFactory version if you called it with a YAML::Node arg first). I also tried * as an option (i.e., as a Any type), but didn't think it was as intuitive (the _ is meant to be like std::placeholder::_1). Another option might be ConfigT but I was a little nervous about that being confused for an instantiable type. I could also drop the extra arg entirely from the signature, but then you get two identical signatures with different modules next to each other which is also confusing
| @@ -0,0 +1,119 @@ | |||
| # Loading External Factories | |||
| instances_.clear(); | ||
| } | ||
|
|
||
| LibraryGuard ExternalRegistry::load(const std::filesystem::path& library_path) { |
There was a problem hiding this comment.
Would it make sense / is it feasible to check whether the library to be loaded exists and is found by LD_PATH? That would allow importing libraries on a 'as available' basis, or warn the user why the library could not be found?
There was a problem hiding this comment.
It might have to be done by a try / catch instead of preemptively checking, but yeah that's a good point (and I should actually be handling boost failing here)
uses member method instead of reimplementing Co-authored-by: Lukas Schmid <[email protected]>
Co-authored-by: Lukas Schmid <[email protected]>
@Schmluk Finally clean enough to PR and review