szaszm commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r605576191
##########
File path: libminifi/test/unit/PropertyTests.cpp
##########
@@ -52,18 +52,6 @@ bool conversionTest(uint64_t number, core::TimeUnit unit,
uint64_t check, Conver
return returnStatus && out == check;
}
-TEST_CASE("Test Boolean Conversion", "[testboolConversion]") {
- bool b;
- REQUIRE(true ==
org::apache::nifi::minifi::utils::StringUtils::StringToBool("true", b));
- REQUIRE(true ==
org::apache::nifi::minifi::utils::StringUtils::StringToBool("True", b));
- REQUIRE(true ==
org::apache::nifi::minifi::utils::StringUtils::StringToBool("TRue", b));
- REQUIRE(true ==
org::apache::nifi::minifi::utils::StringUtils::StringToBool("tRUE", b));
- REQUIRE(false ==
org::apache::nifi::minifi::utils::StringUtils::StringToBool("FALSE", b));
- REQUIRE(false ==
org::apache::nifi::minifi::utils::StringUtils::StringToBool("FALLSEY", b));
- REQUIRE(false ==
org::apache::nifi::minifi::utils::StringUtils::StringToBool("FaLSE", b));
- REQUIRE(false ==
org::apache::nifi::minifi::utils::StringUtils::StringToBool("false", b));
-}
Review comment:
Please convert these tests instead of removing them
##########
File path: libminifi/src/c2/ControllerSocketProtocol.cpp
##########
@@ -51,18 +49,13 @@ void
ControllerSocketProtocol::initialize(core::controller::ControllerServicePro
}
if (nullptr == secure_context) {
std::string secureStr;
- bool is_secure = false;
- if (configuration->get(Configure::nifi_remote_input_secure, secureStr) &&
org::apache::nifi::minifi::utils::StringUtils::StringToBool(secureStr,
is_secure)) {
+ if (configuration->get(Configure::nifi_remote_input_secure, secureStr) &&
org::apache::nifi::minifi::utils::StringUtils::toBool(secureStr).value_or(false))
{
secure_context =
std::make_shared<minifi::controllers::SSLContextService>("ControllerSocketProtocolSSL",
configuration);
secure_context->onEnable();
}
}
- std::string value;
-
- if (configuration_->get("controller.socket.local.any.interface", limitStr)) {
- utils::StringUtils::StringToBool(limitStr, anyInterface);
- }
+ const bool anyInterface
=(configuration_->get("controller.socket.local.any.interface", limitStr) &&
utils::StringUtils::toBool(limitStr).value_or(false));
Review comment:
formatting: remove the outermost parentheses and ensure that there is 1
space on both sides of the operator `=`.
https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace
##########
File path: extensions/standard-processors/processors/GetTCP.cpp
##########
@@ -130,7 +130,7 @@ void GetTCP::onSchedule(const
std::shared_ptr<core::ProcessContext> &context, co
}
if (context->getProperty(StayConnected.getName(), value)) {
- utils::StringUtils::StringToBool(value, stay_connected_);
+ stay_connected_ = utils::StringUtils::toBool(value).value_or(false);
Review comment:
And it's initialized to `true` on line 122, which means it used to stay
true in the case of an unsuccessful conversion.
##########
File path: extensions/mqtt/processors/PublishMQTT.cpp
##########
@@ -64,7 +64,9 @@ void PublishMQTT::onSchedule(const
std::shared_ptr<core::ProcessContext> &contex
logger_->log_debug("PublishMQTT: max flow segment size [%" PRIu64 "]",
max_seg_size_);
}
value = "";
- if (context->getProperty(Retain.getName(), value) && !value.empty() &&
org::apache::nifi::minifi::utils::StringUtils::StringToBool(value, retain_)) {
+ utils::optional<bool> retain_parsed;
+ if (context->getProperty(Retain.getName(), value) && (retain_parsed =
org::apache::nifi::minifi::utils::StringUtils::toBool(value))) {
+ retain_ = retain_parsed.value();
Review comment:
same as above
##########
File path: extensions/mqtt/processors/AbstractMQTTProcessor.cpp
##########
@@ -85,8 +86,9 @@ void AbstractMQTTProcessor::onSchedule(const
std::shared_ptr<core::ProcessContex
logger_->log_debug("AbstractMQTTProcessor: PassWord [%s]", passWord_);
}
value = "";
- if (context->getProperty(CleanSession.getName(), value) && !value.empty() &&
- org::apache::nifi::minifi::utils::StringUtils::StringToBool(value,
cleanSession_)) {
+ utils::optional<bool> cleanSession_parsed;
+ if (context->getProperty(CleanSession.getName(), value) &&
(cleanSession_parsed =
org::apache::nifi::minifi::utils::StringUtils::toBool(value))) {
+ cleanSession_ = cleanSession_parsed.value();
Review comment:
Calling `value()` on an empty optional will throw, which is most likely
not what we want here. The assignment should only be attempted when the
optional is not empty.
other: Assignment in the condition gives me uneasy feelings. Here's an
alternative involving an [immediately invoked
lambda](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-lambda-init)
and the mutable temporary `value`/`property_value` moved to a smaller scope.
```suggestion
const auto cleanSession_parsed = [this, &] -> utils::optional<bool> {
std::string property_value;
if (!context->getProperty(CleanSession.getName(), value)) return
utils::nullopt;
return utils::StringUtils::toBool(property_value);
}();
if (cleanSession_parsed) {
cleanSession_ = *cleanSession_parsed;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
