[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1424: MINIFICPP-1862 use std::filesystem::path instead of std::string where appropriate

2022-12-01 Thread GitBox 


fgerlits commented on code in PR #1424:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1424#discussion_r1035973706


##
Windows.md:
##
@@ -93,7 +93,7 @@ A basic working CMake configuration can be inferred from the 
`win_build_vs.bat`.
 ```
 mkdir build
 cd build
-cmake -G "Visual Studio 16 2019" -DINSTALLER_MERGE_MODULES=OFF 
-DENABLE_SQL=OFF -DCMAKE_BUILD_TYPE_INIT=Release -DCMAKE_BUILD_TYPE=Release 
-DWIN32=WIN32 -DENABLE_LIBRDKAFKA=OFF -DENABLE_JNI=OFF -DOPENSSL_OFF=OFF 
-DENABLE_COAP=OFF -DUSE_SHARED_LIBS=OFF -DDISABLE_CONTROLLER=ON  
-DBUILD_ROCKSDB=ON -DFORCE_WINDOWS=ON -DUSE_SYSTEM_UUID=OFF 
-DDISABLE_LIBARCHIVE=OFF -DDISABLE_SCRIPTING=ON -DEXCLUDE_BOOST=ON 
-DENABLE_WEL=TRUE -DFAIL_ON_WARNINGS=OFF -DSKIP_TESTS=OFF ..

Review Comment:
   `EXCLUDE_BOOST` should be removed from `win_build_vs.bat`, too



##
extensions/expression-language/tests/ExpressionLanguageTests.cpp:
##
@@ -218,7 +216,7 @@ TEST_CASE("GetFile PutFile dynamic attribute", 
"[expressionLanguageTestGetFilePu
   plan->setProperty(extract_text, 
minifi::processors::ExtractText::Attribute.getName(), "extracted_attr_name");
   plan->addProcessor("LogAttribute", "LogAttribute", 
core::Relationship("success", "description"), true);
   auto put_file = plan->addProcessor("PutFile", "PutFile", 
core::Relationship("success", "description"), true);
-  plan->setProperty(put_file, 
minifi::processors::PutFile::Directory.getName(), out_dir + 
"/${extracted_attr_name}");
+  plan->setProperty(put_file, 
minifi::processors::PutFile::Directory.getName(), out_dir.string() + 
"/${extracted_attr_name}");

Review Comment:
   ```suggestion
 plan->setProperty(put_file, 
minifi::processors::PutFile::Directory.getName(), (out_dir / 
"${extracted_attr_name}").string());
   ```



##
extensions/http-curl/tests/C2VerifyServeResults.cpp:
##
@@ -82,8 +82,8 @@ class VerifyC2Server : public HTTPIntegrationBase {
   }
 
  protected:
-  std::string dir;
-  std::stringstream ss;
+  std::filesystem::path dir_;
+  std::filesystem::path path_;

Review Comment:
   I think `test_file_` would be a better name than `path_`



##
extensions/http-curl/tests/HttpPostIntegrationTest.cpp:
##
@@ -78,9 +78,9 @@ class HttpTestHarness : public HTTPIntegrationBase {
   }
 
  protected:
-  std::string dir;
-  std::stringstream ss;
-  TestController testController;
+  std::filesystem::path dir_;
+  std::filesystem::path path_;

Review Comment:
   here, too, I would call this `test_file_` (and everywhere where we currently 
have a `std::filesystem::path path_` field which contains the location of a 
test file)



##
extensions/script/python/PythonCreator.h:
##
@@ -140,26 +136,18 @@ class PythonCreator : public minifi::core::CoreComponent {
 return python_package;
   }
 
-  std::string getPath(const std::string &pythonscript) {
-return std::filesystem::path(pythonscript).parent_path().string();
+  std::filesystem::path getFileName(const std::filesystem::path& 
python_script) {
+return std::filesystem::path(python_script).filename();

Review Comment:
   this conversion is no longer needed:
   ```suggestion
   return python_script.filename();
   ```
   
   Even better, we could remove `getFileName()` and `getScriptName()` and call 
`filename()` and `stem()` instead.



##
extensions/jni/jvm/JniReferenceObjects.h:
##
@@ -244,23 +228,24 @@ class JniSession : public core::WeakReference {
   }
 
   bool prune() {
-global_ff_objects_.erase(std::remove_if(global_ff_objects_.begin(), 
global_ff_objects_.end(), check_empty_ff()), global_ff_objects_.end());
+const auto check_empty_ff = [](const std::shared_ptr& 
flow_file) { return flow_file->empty(); };
+global_ff_objects_.erase(std::remove_if(global_ff_objects_.begin(), 
global_ff_objects_.end(), check_empty_ff), global_ff_objects_.end());

Review Comment:
   this could be simplified with range-v3:
   ```suggestion
   ranges::remove_if(global_ff_objects_, [](const 
std::shared_ptr& flow_file) { return flow_file->empty(); });
   ```
   
   (with an `#include "range/v3/algorithm/remove_if.hpp"`)



##
extensions/script/tests/TestExecuteScriptProcessorWithLuaScript.cpp:
##
@@ -421,8 +412,10 @@ TEST_CASE("Lua: Test Module Directory property", 
"[executescriptLuaModuleDirecto
   true);
 
   plan->setProperty(executeScript, 
minifi::processors::ExecuteScript::ScriptEngine.getName(), "lua");
-  plan->setProperty(executeScript, 
minifi::processors::ExecuteScript::ScriptFile.getName(), 
getScriptFullPath("foo_bar_processor.lua"));
-  plan->setProperty(executeScript, 
minifi::processors::ExecuteScript::ModuleDirectory.getName(), 
getScriptFullPath("foo_modules/foo.lua") + "," + 
getScriptFullPath("bar_modules"));
+  plan->setProperty(executeScript, 
minifi::processors::ExecuteScript::ScriptFile.getName(), script_files_directory 
/ "foo_bar_processor.lua");
+  plan->se

[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1424: MINIFICPP-1862 use std::filesystem::path instead of std::string where appropriate

2022-12-05 Thread GitBox 


fgerlits commented on code in PR #1424:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1424#discussion_r1039600994


##
extensions/standard-processors/tests/unit/TailFileTests.cpp:
##
@@ -99,18 +89,16 @@ TEST_CASE("TailFile reads the file until the first 
delimiter", "[simple]") {
   plan->addProcessor("LogAttribute", "logattribute", 
core::Relationship("success", "description"), true);
 
   auto dir = testController.createTempDirectory();
-  std::stringstream temp_file;
-  temp_file << dir << utils::file::get_separator() << TMP_FILE;
+  auto temp_file_path = dir / TMP_FILE;
 
   std::ofstream tmpfile;
-  tmpfile.open(temp_file.str(), std::ios::out | std::ios::binary);
+  tmpfile.open(temp_file_path, std::ios::out | std::ios::binary);
   tmpfile << NEWLINE_FILE;
   tmpfile.close();
 
-  std::stringstream state_file;
-  state_file << dir << utils::file::get_separator() << STATE_FILE;
+  auto state_file_path = dir / STATE_FILE;

Review Comment:
   this is unused, could be removed



##
libminifi/test/rocksdb-tests/RocksDBTests.cpp:
##
@@ -232,9 +231,9 @@ void 
withDefaultEnv(minifi::internal::Writable& db_opts) {
 }
 
 TEST_CASE_METHOD(RocksDBTest, "Error is logged if different encryption keys 
are used", "[rocksDBTest10]") {
-  utils::Path home_dir{createTempDirectory()};
-  utils::file::FileUtils::create_dir((home_dir / "conf").str());
-  std::ofstream{(home_dir / "conf" / "bootstrap.conf").str()}
+  auto home_dir = createTempDirectory();
+  utils::file::FileUtils::create_dir((home_dir / "conf"));
+  std::ofstream{(home_dir / "conf" / "bootstrap.conf")}

Review Comment:
   parens could be removed here, too



##
minifi_main/MainHelper.cpp:
##
@@ -133,43 +128,41 @@ std::string determineMinifiHome(const 
std::shared_ptr& logger)
 return "";
   }();
 
-  if (minifiHome.empty()) {
+  if (minifi_home.empty()) {
 logger->log_error("No " MINIFI_HOME_ENV_KEY " could be inferred. "
   "Please set " MINIFI_HOME_ENV_KEY " or run minifi from a 
valid location.");
 return "";
   }
 
   /* Verify that MINIFI_HOME is valid */
-  bool minifiHomeValid = false;
-  if (validHome(minifiHome)) {
-minifiHomeValid = true;
+  bool minifi_home_is_valid = false;
+  if (validHome(minifi_home)) {
+minifi_home_is_valid = true;
   } else {
-logger->log_info("%s is not a valid " MINIFI_HOME_ENV_KEY ", because there 
is no " DEFAULT_NIFI_PROPERTIES_FILE " file in it.", minifiHome);
-
-std::string minifiHomeWithoutBin;
-std::string binDir;
-std::tie(minifiHomeWithoutBin, binDir) = 
minifi::utils::file::split_path(minifiHome);
-if (!minifiHomeWithoutBin.empty() && (binDir == "bin" || binDir == 
std::string("bin") + minifi::utils::file::get_separator())) {
-  if (validHome(minifiHomeWithoutBin)) {
-logger->log_info("%s is a valid " MINIFI_HOME_ENV_KEY ", falling back 
to it.", minifiHomeWithoutBin);
-minifiHomeValid = true;
-minifiHome = std::move(minifiHomeWithoutBin);
+logger->log_info("%s is not a valid %s, because there is no %s file in 
it.", minifi_home.string(), MINIFI_HOME_ENV_KEY, 
DEFAULT_NIFI_PROPERTIES_FILE.string());
+
+auto minifi_home_without_bin = minifi_home.parent_path();
+auto bin_dir = minifi_home.filename();
+if (!minifi_home_without_bin.empty() && (bin_dir == 
std::filesystem::path("bin") || bin_dir == (std::filesystem::path("bin")/""))) {

Review Comment:
   I don't think it's possible for `minifi_home.filename()` to end in a 
separator, so
   ```suggestion
   if (!minifi_home_without_bin.empty() && bin_dir == 
std::filesystem::path("bin")) {
   ```



##
libminifi/include/core/ConfigurableComponent.h:
##
@@ -59,6 +59,18 @@ class ConfigurableComponent {
   template
   bool getProperty(const std::string name, T &value) const;
 
+  template
+  std::enable_if_t::value, std::optional>
+  getProperty(const std::string& property_name) const {

Review Comment:
   can we use concepts now?
   ```suggestion
 template
 std::optional getProperty(const std::string& property_name) const {
   ```
   would be nicer



##
libminifi/test/rocksdb-tests/EncryptionTests.cpp:
##
@@ -77,8 +75,8 @@ TEST_CASE_METHOD(FFRepoFixture, "FlowFileRepository creates 
checkpoint and loads
 // pass
   }
   SECTION("With encryption") {
-utils::file::FileUtils::create_dir((home_ / "conf").str());
-std::ofstream{(home_ / "conf" / "bootstrap.conf").str()}
+utils::file::FileUtils::create_dir((home_ / "conf"));
+std::ofstream{(home_ / "conf" / "bootstrap.conf")}

Review Comment:
   nitpicking, but these parens are not needed any longer:
   ```suggestion
   utils::file::FileUtils::create_dir(home_ / "conf");
   std::ofstream{home_ / "conf" / "bootstrap.conf"}
   ```



##
libminifi/src/core/ProcessSessionReadCallback.cpp:
##
@@ -22,18 +22,19 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "core/logging/Log

[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1424: MINIFICPP-1862 use std::filesystem::path instead of std::string where appropriate

2022-12-09 Thread GitBox 


fgerlits commented on code in PR #1424:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1424#discussion_r1044659717


##
libminifi/test/rocksdb-tests/EncryptionTests.cpp:
##
@@ -77,8 +75,8 @@ TEST_CASE_METHOD(FFRepoFixture, "FlowFileRepository creates 
checkpoint and loads
 // pass
   }
   SECTION("With encryption") {
-utils::file::FileUtils::create_dir((home_ / "conf").str());
-std::ofstream{(home_ / "conf" / "bootstrap.conf").str()}
+utils::file::FileUtils::create_dir((home_ / "conf"));
+std::ofstream{(home_ / "conf" / "bootstrap.conf")}

Review Comment:
   the double parens in line 78 are still there; are those needed?



-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1424: MINIFICPP-1862 use std::filesystem::path instead of std::string where appropriate

2022-12-09 Thread GitBox 


fgerlits commented on code in PR #1424:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1424#discussion_r1044660504


##
libminifi/test/rocksdb-tests/RocksDBTests.cpp:
##
@@ -232,9 +231,9 @@ void 
withDefaultEnv(minifi::internal::Writable& db_opts) {
 }
 
 TEST_CASE_METHOD(RocksDBTest, "Error is logged if different encryption keys 
are used", "[rocksDBTest10]") {
-  utils::Path home_dir{createTempDirectory()};
-  utils::file::FileUtils::create_dir((home_dir / "conf").str());
-  std::ofstream{(home_dir / "conf" / "bootstrap.conf").str()}
+  auto home_dir = createTempDirectory();
+  utils::file::FileUtils::create_dir((home_dir / "conf"));
+  std::ofstream{(home_dir / "conf" / "bootstrap.conf")}

Review Comment:
   same here; line 235 still has the double parens



-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1424: MINIFICPP-1862 use std::filesystem::path instead of std::string where appropriate

2022-12-09 Thread GitBox 


fgerlits commented on code in PR #1424:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1424#discussion_r1044661369


##
libminifi/test/unit/FileStreamTests.cpp:
##
@@ -301,32 +267,25 @@ TEST_CASE("Existing file read/write test") {
   stream.seek(0);
 }
 
-#if !defined(WIN32) || defined(USE_BOOST)
-// This could be simplified with C++17 std::filesystem
 TEST_CASE("Opening file without permission creates error logs") {
   TestController test_controller;
   auto dir = test_controller.createTempDirectory();
-  std::string path_to_permissionless_file(utils::file::concat_path(dir, 
"permissionless_file.txt"));
+  auto path_to_permissionless_file = dir / "permissionless_file.txt";
   {
 std::ofstream outfile(path_to_permissionless_file);
 outfile << "this file has been just created" << std::endl;
 outfile.close();
-#ifndef WIN32
 utils::file::FileUtils::set_permissions(path_to_permissionless_file, 0);
-#else
-boost::filesystem::permissions(path_to_permissionless_file, 
boost::filesystem::no_perms);
-#endif
   }
-  minifi::io::FileStream stream(path_to_permissionless_file, 0, false);
+  minifi::io::FileStream stream(path_to_permissionless_file, 0, true);

Review Comment:
   thanks, makes sense
   
   can you add this as a comment here, please?



-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1424: MINIFICPP-1862 use std::filesystem::path instead of std::string where appropriate

2022-12-09 Thread GitBox 


fgerlits commented on code in PR #1424:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1424#discussion_r1044702728


##
extensions/sftp/tests/ListThenFetchSFTPTests.cpp:
##
@@ -121,7 +120,7 @@ class ListThenFetchSFTPTestsFixture {
 plan->setProperty(list_sftp, "Minimum File Size", "0 B");
 plan->setProperty(list_sftp, "Target System Timestamp Precision", 
"Seconds");
 plan->setProperty(list_sftp, "Remote Path", "nifi_test/");
-plan->setProperty(list_sftp, "State File", src_dir + "/state");
+plan->setProperty(list_sftp, "State File", src_dir.string() + "/state");

Review Comment:
   why not `(src_dir / "state").string()`?  (at line 142, too)



-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1424: MINIFICPP-1862 use std::filesystem::path instead of std::string where appropriate

2022-12-12 Thread GitBox 


fgerlits commented on code in PR #1424:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1424#discussion_r1045813064


##
extensions/sftp/tests/ListSFTPTests.cpp:
##
@@ -187,7 +180,7 @@ TEST_CASE_METHOD(ListSFTPTestsFixture, "ListSFTP list one 
file", "[ListSFTP][bas
 
 TEST_CASE_METHOD(ListSFTPTestsFixture, "ListSFTP public key authentication", 
"[ListSFTP][basic]") {
   plan->setProperty(list_sftp, "Remote File", "nifi_test/tstFile.ext");
-  plan->setProperty(list_sftp, "Private Key Path", 
utils::file::FileUtils::concat_path(get_sftp_test_dir(), "resources/id_rsa"));
+  plan->setProperty(list_sftp, "Private Key Path", get_sftp_test_dir() / 
"resources/id_rsa");

Review Comment:
   yes, I agree it's more readable this way



-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org