lordgamez commented on a change in pull request #934:
URL: https://github.com/apache/nifi-minifi-cpp/pull/934#discussion_r519920462
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -22,6 +22,39 @@
#include "../../include/core/Property.h"
#include "utils/StringUtils.h"
#include "../TestBase.h"
+namespace {
Review comment:
Do we need the anonymous namespace here?
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -22,6 +22,39 @@
#include "../../include/core/Property.h"
#include "utils/StringUtils.h"
#include "../TestBase.h"
+namespace {
+enum class ParsingStatus { ParsingFail , ParsingSuccessful , ValuesMatch };
+enum class ConversionTestTarget { MS, NS };
+
+ParsingStatus checkTimeValue(const std::string , int64_t t1,
core::TimeUnit t2) {
+ int64_t TimeVal;
+ core::TimeUnit unit;
+ bool parsing_succeeded =
org::apache::nifi::minifi::core::Property::StringToTime(input, TimeVal, unit);
+ if (parsing_succeeded) {
+if (TimeVal == t1 && unit == t2) {
+ return ParsingStatus::ValuesMatch;
+} else {
+ return ParsingStatus::ParsingSuccessful;
+}
+ } else {
+return ParsingStatus::ParsingFail;
+ }
+}
+
+bool conversionTest(uint64_t number, core::TimeUnit unit, uint64_t check,
ConversionTestTarget conversionUnit) {
+ uint64_t out;
+ bool returnStatus;
Review comment:
Minor: I know these will be eventually filled by the parser function,
but I would recommend giving an initial value for the primitive types.
##
File path: libminifi/include/utils/ValueParser.h
##
@@ -216,6 +216,7 @@ bool StringToTime(const std::string& input, Out& output,
core::TimeUnit& timeuni
}
}
+
Review comment:
Minor: unnecessary ws.
##
File path: libminifi/test/unit/PropertyTests.cpp
##
@@ -22,6 +22,39 @@
#include "../../include/core/Property.h"
#include "utils/StringUtils.h"
#include "../TestBase.h"
+namespace {
+enum class ParsingStatus { ParsingFail , ParsingSuccessful , ValuesMatch };
+enum class ConversionTestTarget { MS, NS };
+
+ParsingStatus checkTimeValue(const std::string , int64_t t1,
core::TimeUnit t2) {
+ int64_t TimeVal;
+ core::TimeUnit unit;
+ bool parsing_succeeded =
org::apache::nifi::minifi::core::Property::StringToTime(input, TimeVal, unit);
+ if (parsing_succeeded) {
+if (TimeVal == t1 && unit == t2) {
+ return ParsingStatus::ValuesMatch;
+} else {
+ return ParsingStatus::ParsingSuccessful;
+}
+ } else {
+return ParsingStatus::ParsingFail;
+ }
+}
+
+bool conversionTest(uint64_t number, core::TimeUnit unit, uint64_t check,
ConversionTestTarget conversionUnit) {
+ uint64_t out;
+ bool returnStatus;
+ if (conversionUnit == ConversionTestTarget::NS) {
+returnStatus =
org::apache::nifi::minifi::core::Property::ConvertTimeUnitToNS(number, unit,
out);
+ } else if (conversionUnit == ConversionTestTarget::MS) {
+returnStatus =
org::apache::nifi::minifi::core::Property::ConvertTimeUnitToMS(number, unit,
out);
+ }
+ if (returnStatus && out == check) {
+return true;
+ } else {
+return false;
+ }
Review comment:
```suggestion
return returnreturnStatus && out == check;
```
##
File path: libminifi/include/core/Property.h
##
@@ -180,10 +180,15 @@ class Property {
// Compare
bool operator <(const Property & right) const;
-// Convert TimeUnit to MilliSecond
Review comment:
:+1: I'm always for the idea of removing trivial comments
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
