[WIP] Refactor, stage 2#113
Conversation
alanking
left a comment
alanking
left a comment
There was a problem hiding this comment.
Lookin real nice so far
| #ifdef __cpp_lib_filesystem | ||
| #include <filesystem> | ||
| namespace fs = std::filesystem; | ||
| #else | ||
| #include <boost/filesystem.hpp> | ||
| namespace fs = boost::filesystem; | ||
| #endif |
There was a problem hiding this comment.
Just curious: Is this feature check for purposes of portability?
There was a problem hiding this comment.
Okay. It made me wonder to what extent we wanted to do things like this. For instance, if this is to support older compilers, do we need to do something similar for, i.e. string_view (>=C++17)? I don't have a problem with this one, was just wondering where we're drawing this particular line, and/or if I missed the point. :)
There was a problem hiding this comment.
It's less about the compiler and more about the stdlib. If you think it's unlikely that someone would compile against a stdlib that would have string_view and not std::filesystem I can remove this
| { | ||
| public: | ||
| amqp_endpoint( | ||
| const std::string_view& _scheme, |
There was a problem hiding this comment.
Any reason these std::string_views are refs?
| namespace | ||
| { | ||
| BOOST_FORCEINLINE std::string build_amqp_url( | ||
| const std::string_view& _scheme, |
There was a problem hiding this comment.
Should we be using refs with std::string_view?
| {"rule_engine_plugin", rule_engine_name}, | ||
| {"instance_name", _instance_name}, | ||
| {"log_message", "Ignoring amqp_location and amqp_topic in favor of amqp_endpoints. These " | ||
| "settings have been deprecated and should be removed from the plugin ." |
There was a problem hiding this comment.
Is the '.' at the end of this line of text intentional?
|
Closing in favor of #185 |
2 participants
Creating a draft PR now since I'm switching gears for a bit.
This is the PR for stage 2 of the refactor started by #105. This stage of the refactor will introduce a dedicated thread for the AMQP message sender and the ability to handle multiple AMQP endpoints.