[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
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
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
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
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
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
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
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