[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r630852673
##
File path: controller/MiNiFiController.cpp
##
@@ -167,9 +165,10 @@ int main(int argc, char **argv) {
socket = secure_context != nullptr ?
stream_factory_->createSecureSocket(host, port, secure_context) :
stream_factory_->createSocket(host, port);
if (getConnectionSize(std::move(socket), std::cout, connection) < 0)
std::cout << "Could not connect to remote host " << host << ":" <<
port << std::endl;
-} else
+} else {
std::cout << "Could not connect to remote host " << host << ":" <<
port << std::endl;
}
+ }
Review comment:
Sorry I thought this is an old comment and didn't see the date.
Hopefully it's correct now.
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r621955763
##
File path: extensions/sftp/processors/SFTPProcessorBase.cpp
##
@@ -180,7 +180,7 @@ void
SFTPProcessorBase::parseCommonPropertiesOnSchedule(const std::shared_ptrgetProperty(SendKeepaliveOnTimeout.getName(), value)) {
logger_->log_error("Send Keep Alive On Timeout attribute is missing or
invalid");
} else {
-utils::StringUtils::StringToBool(value, use_keepalive_on_timeout_);
+use_keepalive_on_timeout_ =
utils::StringUtils::toBool(value).value_or(false);
Review comment:
Fixed :)
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r618944659
##
File path: extensions/standard-processors/processors/HashContent.cpp
##
@@ -84,6 +82,7 @@ void HashContent::onTrigger(core::ProcessContext *,
core::ProcessSession *sessio
}
if (failOnEmpty_ && flowFile->getSize() == 0) {
+logger_->log_trace("Failure as flow file is empty");
Review comment:
Okay. Done :)
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r618943625
##
File path: libminifi/include/utils/StringUtils.h
##
@@ -75,13 +75,10 @@ struct string_traits{
class StringUtils {
public:
/**
- * Converts a string to a boolean
- * Better handles mixed case.
+ * Checks and converts a string to a boolean
* @param input input string
- * @param output output string.
+ * @returns an optional of a boolean: true if the string is "true" (ignoring
case), false if it is "false" (ignoring case), nullopt for any other value
*/
- static bool StringToBool(std::string input, bool );
Review comment:
Yeah I am not sure why I left that part. Anyway, fixed it now :)
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040: URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r618310160 ## File path: docker/test/integration/features/hashcontent.feature ## @@ -0,0 +1,10 @@ +Feature: Adding hash value for a FlowFile using HashContent + In order to calculate a hash value for a FlowFile + As a user of MiNiFi + I need to have HashContent Processor + +Background: + Given the content of "/tmp/output" is monitored + +Scenario: HashContent adds hash attribute to flowfiles + Given a GetFile processor with "Input Directory" property set to "/tmp/input" Review comment: Yeah my bad. Sorry. -- 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: us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r612988788
##
File path: extensions/mqtt/processors/PublishMQTT.cpp
##
@@ -64,7 +64,9 @@ void PublishMQTT::onSchedule(const
std::shared_ptr
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 retain_parsed;
+ if (context->getProperty(Retain.getName(), value) && (retain_parsed =
org::apache::nifi::minifi::utils::StringUtils::toBool(value))) {
+retain_ = retain_parsed.value();
Review comment:
Fixed :)
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r612988521
##
File path: libminifi/include/utils/StringUtils.h
##
@@ -75,13 +75,12 @@ struct string_traits{
class StringUtils {
public:
/**
- * Converts a string to a boolean
- * Better handles mixed case.
+ * Checks and converts a string to a boolean
* @param input input string
- * @param output output string.
+ * @returns an optional of a boolean: true if the string is "true" (ignoring
case), false if it is "false" (ignoring case), nullopt for any other value
*/
- static bool StringToBool(std::string input, bool );
+ static bool StringToBool(std::string input, bool );
Review comment:
Done :)
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r612988411
##
File path: extensions/standard-processors/processors/GetTCP.cpp
##
@@ -130,7 +130,7 @@ void GetTCP::onSchedule(const
std::shared_ptr , co
}
if (context->getProperty(StayConnected.getName(), value)) {
-utils::StringUtils::StringToBool(value, stay_connected_);
+stay_connected_ = utils::StringUtils::toBool(value).value_or(false);
Review comment:
Fixed :)
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r612988235
##
File path: controller/MiNiFiController.cpp
##
@@ -167,9 +165,10 @@ int main(int argc, char **argv) {
socket = secure_context != nullptr ?
stream_factory_->createSecureSocket(host, port, secure_context) :
stream_factory_->createSocket(host, port);
if (getConnectionSize(std::move(socket), std::cout, connection) < 0)
std::cout << "Could not connect to remote host " << host << ":" <<
port << std::endl;
-} else
+} else {
std::cout << "Could not connect to remote host " << host << ":" <<
port << std::endl;
}
+ }
Review comment:
Fixed :)
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r608373420
##
File path: extensions/standard-processors/processors/HashContent.cpp
##
@@ -84,6 +82,7 @@ void HashContent::onTrigger(core::ProcessContext *,
core::ProcessSession *sessio
}
if (failOnEmpty_ && flowFile->getSize() == 0) {
+logger_->log_trace("Failure as flow file is empty");
Review comment:
Yeah I added it to checks logs in testing. How do you suggest I tweak
this?
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040: URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r608365026 ## File path: extensions/mqtt/processors/AbstractMQTTProcessor.cpp ## @@ -146,7 +148,7 @@ void AbstractMQTTProcessor::onSchedule(const std::shared_ptr (this), connectionLost, msgReceived, msgDelivered); Review comment: Yeah it was the C-style cast. I removed casting from here as Feri said implicit conversion was enough in this case. -- 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: us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040: URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r608365026 ## File path: extensions/mqtt/processors/AbstractMQTTProcessor.cpp ## @@ -146,7 +148,7 @@ void AbstractMQTTProcessor::onSchedule(const std::shared_ptr (this), connectionLost, msgReceived, msgDelivered); Review comment: Yeah it was the C-style cast. I removed casting from here as Feri said implicit conversion was enough. -- 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: us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r605784868
##
File path: libminifi/test/KamikazeProcessor.cpp
##
@@ -53,14 +53,13 @@ void KamikazeProcessor::initialize() {
void KamikazeProcessor::onSchedule(core::ProcessContext *context,
core::ProcessSessionFactory* /*sessionFactory*/) {
std::string value;
- bool bool_value;
-
_throwInOnTrigger = false;
Review comment:
oh okay. Sorry I got it wrong the first time.
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040: URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r605759075 ## File path: extensions/mqtt/processors/AbstractMQTTProcessor.cpp ## @@ -146,7 +148,7 @@ void AbstractMQTTProcessor::onSchedule(const std::shared_ptr (this), connectionLost, msgReceived, msgDelivered); Review comment: Yeah it's not needed but it was producing a linter warning in earlier case. Should I revert the change back? -- 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: us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r605747978
##
File path: extensions/http-curl/tests/unit/InvokeHTTPTests.cpp
##
@@ -37,12 +37,12 @@
#include "core/ProcessSession.h"
#include "core/ProcessorNode.h"
#include "processors/InvokeHTTP.h"
-#include "processors/ListenHTTP.h"
#include "processors/LogAttribute.h"
#include "utils/gsl.h"
TEST_CASE("HTTPTestsWithNoResourceClaimPOST", "[httptest1]") {
TestController testController;
+ // TODO(aminadinari19): fix this test
Review comment:
It's not registered in ctest and produces an error if I try to run it. I
made a Jira for it too. https://issues.apache.org/jira/browse/MINIFICPP-1510
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r605742518
##
File path: extensions/standard-processors/processors/GetTCP.cpp
##
@@ -130,7 +130,7 @@ void GetTCP::onSchedule(const
std::shared_ptr , co
}
if (context->getProperty(StayConnected.getName(), value)) {
-utils::StringUtils::StringToBool(value, stay_connected_);
+stay_connected_ = utils::StringUtils::toBool(value).value_or(false);
Review comment:
So my takeaway is that a default value is not really needed here.
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r605627557
##
File path: extensions/standard-processors/processors/GetTCP.cpp
##
@@ -130,7 +130,7 @@ void GetTCP::onSchedule(const
std::shared_ptr , co
}
if (context->getProperty(StayConnected.getName(), value)) {
-utils::StringUtils::StringToBool(value, stay_connected_);
+stay_connected_ = utils::StringUtils::toBool(value).value_or(false);
Review comment:
Quick question: Do we need else branch here?
--
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1040: MINIFICPP-1329- Fix implementation and usages of string to bool
aminadinari19 commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r605587832
##
File path: libminifi/test/KamikazeProcessor.cpp
##
@@ -53,14 +53,13 @@ void KamikazeProcessor::initialize() {
void KamikazeProcessor::onSchedule(core::ProcessContext *context,
core::ProcessSessionFactory* /*sessionFactory*/) {
std::string value;
- bool bool_value;
-
_throwInOnTrigger = false;
Review comment:
It's already deleted I guess
--
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:
us...@infra.apache.org
