lordgamez commented on code in PR #1409:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1409#discussion_r963729273
##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -186,7 +186,7 @@ ListSFTP::Child::Child(const std::string& parent_path_,
std::tuple<std::string /
}
std::string ListSFTP::Child::getPath() const {
- return utils::file::FileUtils::concat_path(parent_path, filename, true
/*force_posix*/);
+ return utils::StringUtils::join_pack(parent_path, "/", filename);
Review Comment:
Should we maybe add an alias for posix concatenation in the fileutils, that
would call the join_pack this way? I think that would be a bit more explicit.
##########
libminifi/include/Defaults.h:
##########
@@ -18,15 +18,15 @@
#pragma once
#ifdef WIN32
-#define DEFAULT_NIFI_CONFIG_YML "\\conf\\config.yml"
-#define DEFAULT_NIFI_PROPERTIES_FILE "\\conf\\minifi.properties"
-#define DEFAULT_LOG_PROPERTIES_FILE "\\conf\\minifi-log.properties"
-#define DEFAULT_UID_PROPERTIES_FILE "\\conf\\minifi-uid.properties"
-#define DEFAULT_BOOTSTRAP_FILE "\\conf\\bootstrap.conf"
+#define DEFAULT_NIFI_CONFIG_YML "conf\\config.yml"
+#define DEFAULT_NIFI_PROPERTIES_FILE "conf\\minifi.properties"
+#define DEFAULT_LOG_PROPERTIES_FILE "conf\\minifi-log.properties"
+#define DEFAULT_UID_PROPERTIES_FILE "conf\\minifi-uid.properties"
+#define DEFAULT_BOOTSTRAP_FILE "conf\\bootstrap.conf"
#else
-#define DEFAULT_NIFI_CONFIG_YML "./conf/config.yml"
-#define DEFAULT_NIFI_PROPERTIES_FILE "./conf/minifi.properties"
-#define DEFAULT_LOG_PROPERTIES_FILE "./conf/minifi-log.properties"
-#define DEFAULT_UID_PROPERTIES_FILE "./conf/minifi-uid.properties"
-#define DEFAULT_BOOTSTRAP_FILE "./conf/bootstrap.conf"
+#define DEFAULT_NIFI_CONFIG_YML "conf/config.yml"
+#define DEFAULT_NIFI_PROPERTIES_FILE "conf/minifi.properties"
+#define DEFAULT_LOG_PROPERTIES_FILE "conf/minifi-log.properties"
+#define DEFAULT_UID_PROPERTIES_FILE "conf/minifi-uid.properties"
+#define DEFAULT_BOOTSTRAP_FILE "conf/bootstrap.conf"
Review Comment:
Could we replace these with `constexpr const char*` instead of defines?
##########
libminifi/test/unit/FileUtilsTests.cpp:
##########
@@ -52,43 +52,43 @@ TEST_CASE("TestFileUtils::concat_path", "[TestConcatPath]")
{
TEST_CASE("TestFileUtils::get_parent_path", "[TestGetParentPath]") {
#ifdef WIN32
- REQUIRE("foo\\" == FileUtils::get_parent_path("foo\\bar"));
- REQUIRE("foo\\" == FileUtils::get_parent_path("foo\\bar\\"));
- REQUIRE("C:\\foo\\" == FileUtils::get_parent_path("C:\\foo\\bar"));
- REQUIRE("C:\\foo\\" == FileUtils::get_parent_path("C:\\foo\\bar\\"));
- REQUIRE("C:\\" == FileUtils::get_parent_path("C:\\foo"));
- REQUIRE("C:\\" == FileUtils::get_parent_path("C:\\foo\\"));
- REQUIRE("" == FileUtils::get_parent_path("C:\\")); //
NOLINT(readability-container-size-empty)
- REQUIRE("" == FileUtils::get_parent_path("C:\\\\")); //
NOLINT(readability-container-size-empty)
+ CHECK("foo" == FileUtils::get_parent_path("foo\\bar"));
+ CHECK("foo\\bar" == FileUtils::get_parent_path("foo\\bar\\"));
Review Comment:
I think this is a bit confusing that both refer to the `foo\\bar` directory
but they have a different result when calling `get_parent_path`. get_child_path
has a similar problem. Either the functionality should be changed or the
current functionality should be better documented (in comments) and the better
represented in the name of the functions.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
