[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1112: MINIFICPP-1567 enable linter checks in extensions (part 4)

[GitBox <gi...@apache.org>]

fgerlits commented on a change in pull request #1112:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1112#discussion_r661617956



##########
File path: extensions/rocksdb-repos/DatabaseContentRepository.h
##########
@@ -38,7 +40,7 @@ namespace repository {
 class StringAppender : public rocksdb::AssociativeMergeOperator {
  public:
   // Constructor: specify delimiter
-  explicit StringAppender() = default;
+  StringAppender() = default;

Review comment:
       minor, but this comment looks stale; I would remove both lines, as the compiler will generate a default constructor anyway

##########
File path: extensions/rocksdb-repos/ProvenanceRepository.h
##########
@@ -156,7 +162,7 @@ class ProvenanceRepository : public core::Repository, public std::enable_shared_
       std::string key = it->key().ToString();
       if (store.size() >= max_size)
         break;
-      if (eventRead->DeSerialize((uint8_t *) it->value().data(), (int) it->value().size())) {
+      if (eventRead->DeSerialize(reinterpret_cast<const uint8_t *>(it->value().data()), static_cast<int>(it->value().size()))) {

Review comment:
       this cast to `int` looks dangerous (we may be silently dropping the high-order bits), and it is also unnecessary, as the second parameter of `DeSerialize()` is of type `size_t` [also in a few places further down in this file]
   ```suggestion
         if (eventRead->DeSerialize(reinterpret_cast<const uint8_t *>(it->value().data()), it->value().size())) {
   ```

##########
File path: extensions/sensors/SensorBase.h
##########
@@ -64,17 +61,17 @@ class SensorBase : public core::Processor {
   void onSchedule(const std::shared_ptr<core::ProcessContext> &context, const std::shared_ptr<core::ProcessSessionFactory> &sessionFactory) override;
 
   class WriteCallback : public OutputStreamCallback {
-     public:
-      explicit WriteCallback(std::string data)
-          : data_{std::move(data)}
-      {}
-      std::string data_;
-      int64_t process(const std::shared_ptr<io::BaseStream>& stream) override {
-        if (data_.empty()) return 0;
-        const auto write_ret = stream->write(reinterpret_cast<const uint8_t*>(data_.data()), data_.size());
-        return io::isError(write_ret) ? -1 : gsl::narrow<int64_t>(write_ret);
-      }
-    };
+   public:
+    explicit WriteCallback(std::string data)
+        : data_{std::move(data)} {
+    }
+    std::string data_;
+    int64_t process(const std::shared_ptr<io::BaseStream>& stream) override {
+      if (data_.empty()) return 0;
+      const auto write_ret = stream->write(reinterpret_cast<const uint8_t*>(data_.data()), data_.size());
+      return io::isError(write_ret) ? -1 : gsl::narrow<int64_t>(write_ret);
+    }
+};

Review comment:
       this should be indented; I would also add a blank line after it
   ```suggestion
     };
   
   ```

##########
File path: extensions/sftp/tests/FetchSFTPTests.cpp
##########
@@ -142,7 +142,7 @@ class FetchSFTPTestsFixture {
     std::fstream file;
     std::stringstream ss;
     ss << src_dir << "/vfs/" << relative_path;
-    utils::file::FileUtils::create_dir(utils::file::FileUtils::get_parent_path(ss.str()));  // TODO
+    utils::file::FileUtils::create_dir(utils::file::FileUtils::get_parent_path(ss.str()));  // TODO(_)

Review comment:
       I doubt anyone knows what this TODO refers to, so I think we should remove it rather than make it cpplint-compliant
   
   also, `utils::file::FileUtils::` is the same as `utils::file::`, so we can remove `FileUtils`; maybe that is what the TODO was about?
   
   ```suggestion
       utils::file::create_dir(utils::file::get_parent_path(ss.str()));
   ```

##########
File path: extensions/script/python/PyProcCreator.h
##########
@@ -56,6 +58,7 @@ class PythonObjectFactory : public core::DefautObjectFactory<minifi::python::pro
   /**
    * Create a shared pointer to a new processor.
    */
+

Review comment:
       very minor, but why did you add this empty line?
   
   I would remove it, and I would also remove the comment, and all four doxygen comments from the `create()` functions, as they are 50% incorrect and 100% unnecessary

##########
File path: extensions/sftp/processors/SFTPProcessorBase.cpp
##########
@@ -48,59 +48,74 @@ namespace nifi {
 namespace minifi {
 namespace processors {
 
-core::Property SFTPProcessorBase::Hostname(
-    core::PropertyBuilder::createProperty("Hostname")->withDescription("The fully qualified hostname or IP address of the remote system")
-        ->isRequired(true)->supportsExpressionLanguage(true)->build());
-core::Property SFTPProcessorBase::Port(
-    core::PropertyBuilder::createProperty("Port")->withDescription("The port that the remote system is listening on for file transfers")
-        ->isRequired(true)->supportsExpressionLanguage(true)->build());
-core::Property SFTPProcessorBase::Username(
-    core::PropertyBuilder::createProperty("Username")->withDescription("Username")
-        ->isRequired(true)->supportsExpressionLanguage(true)->build());
-core::Property SFTPProcessorBase::Password(
-    core::PropertyBuilder::createProperty("Password")->withDescription("Password for the user account")
-        ->isRequired(false)->supportsExpressionLanguage(true)->build());
-core::Property SFTPProcessorBase::PrivateKeyPath(
-    core::PropertyBuilder::createProperty("Private Key Path")->withDescription("The fully qualified path to the Private Key file")
-        ->isRequired(false)->supportsExpressionLanguage(true)->build());
-core::Property SFTPProcessorBase::PrivateKeyPassphrase(
-    core::PropertyBuilder::createProperty("Private Key Passphrase")->withDescription("Password for the private key")
-        ->isRequired(false)->supportsExpressionLanguage(true)->build());
-core::Property SFTPProcessorBase::StrictHostKeyChecking(
-    core::PropertyBuilder::createProperty("Strict Host Key Checking")->withDescription("Indicates whether or not strict enforcement of hosts keys should be applied")
-        ->isRequired(true)->withDefaultValue<bool>(false)->build());
-core::Property SFTPProcessorBase::HostKeyFile(
-    core::PropertyBuilder::createProperty("Host Key File")->withDescription("If supplied, the given file will be used as the Host Key; otherwise, no use host key file will be used")
-        ->isRequired(false)->build());
-core::Property SFTPProcessorBase::ConnectionTimeout(
-    core::PropertyBuilder::createProperty("Connection Timeout")->withDescription("Amount of time to wait before timing out while creating a connection")
-        ->isRequired(true)->withDefaultValue<core::TimePeriodValue>("30 sec")->build());
-core::Property SFTPProcessorBase::DataTimeout(
-    core::PropertyBuilder::createProperty("Data Timeout")->withDescription("When transferring a file between the local and remote system, this value specifies how long is allowed to elapse without any data being transferred between systems")
-        ->isRequired(true)->withDefaultValue<core::TimePeriodValue>("30 sec")->build());
-core::Property SFTPProcessorBase::SendKeepaliveOnTimeout(
-    core::PropertyBuilder::createProperty("Send Keep Alive On Timeout")->withDescription("Indicates whether or not to send a single Keep Alive message when SSH socket times out")
-        ->isRequired(true)->withDefaultValue<bool>(true)->build());
-core::Property SFTPProcessorBase::ProxyType(
-    core::PropertyBuilder::createProperty("Proxy Type")->withDescription("Specifies the Proxy Configuration Controller Service to proxy network requests. If set, it supersedes proxy settings configured per component. "
-                                                                         "Supported proxies: HTTP + AuthN, SOCKS + AuthN")
-        ->isRequired(false)
-        ->withAllowableValues<std::string>({PROXY_TYPE_DIRECT,
+core::Property SFTPProcessorBase::Hostname(core::PropertyBuilder::createProperty("Hostname")
+    ->withDescription("The fully qualified hostname or IP address of the remote system")
+    ->isRequired(true)->supportsExpressionLanguage(true)->build());
+
+core::Property SFTPProcessorBase::Port(core::PropertyBuilder::createProperty("Port")
+    ->withDescription("The port that the remote system is listening on for file transfers")
+    ->isRequired(true)->supportsExpressionLanguage(true)->build());
+
+core::Property SFTPProcessorBase::Username(core::PropertyBuilder::createProperty("Username")
+    ->withDescription("Username")
+    ->isRequired(true)->supportsExpressionLanguage(true)->build());
+
+core::Property SFTPProcessorBase::Password(core::PropertyBuilder::createProperty("Password")
+    ->withDescription("Password for the user account")
+    ->isRequired(false)->supportsExpressionLanguage(true)->build());
+
+core::Property SFTPProcessorBase::PrivateKeyPath(core::PropertyBuilder::createProperty("Private Key Path")
+    ->withDescription("The fully qualified path to the Private Key file")
+    ->isRequired(false)->supportsExpressionLanguage(true)->build());
+
+core::Property SFTPProcessorBase::PrivateKeyPassphrase(core::PropertyBuilder::createProperty("Private Key Passphrase")
+    ->withDescription("Password for the private key")
+    ->isRequired(false)->supportsExpressionLanguage(true)->build());
+
+core::Property SFTPProcessorBase::StrictHostKeyChecking(core::PropertyBuilder::createProperty("Strict Host Key Checking")
+    ->withDescription("Indicates whether or not strict enforcement of hosts keys should be applied")
+    ->isRequired(true)->withDefaultValue<bool>(false)->build());
+
+core::Property SFTPProcessorBase::HostKeyFile(core::PropertyBuilder::createProperty("Host Key File")
+    ->withDescription("If supplied, the given file will be used as the Host Key; otherwise, no use host key file will be used")
+    ->isRequired(false)->build());
+
+core::Property SFTPProcessorBase::ConnectionTimeout(core::PropertyBuilder::createProperty("Connection Timeout")
+    ->withDescription("Amount of time to wait before timing out while creating a connection")
+    ->isRequired(true)->withDefaultValue<core::TimePeriodValue>("30 sec")->build());
+
+core::Property SFTPProcessorBase::DataTimeout(core::PropertyBuilder::createProperty("Data Timeout")
+    ->withDescription("When transferring a file between the local and remote system, this value specifies how long is allowed to elapse without any data being transferred between systems")
+    ->isRequired(true)->withDefaultValue<core::TimePeriodValue>("30 sec")->build());
+
+core::Property SFTPProcessorBase::SendKeepaliveOnTimeout(core::PropertyBuilder::createProperty("Send Keep Alive On Timeout")
+    ->withDescription("Indicates whether or not to send a single Keep Alive message when SSH socket times out")
+    ->isRequired(true)->withDefaultValue<bool>(true)->build());
+
+core::Property SFTPProcessorBase::ProxyType(core::PropertyBuilder::createProperty("Proxy Type")
+    ->withDescription("Specifies the Proxy Configuration Controller Service to proxy network requests. If set, it supersedes proxy settings configured per component. "
+                       "Supported proxies: HTTP + AuthN, SOCKS + AuthN")
+    ->isRequired(false)
+    ->withAllowableValues<std::string>({PROXY_TYPE_DIRECT,
                                             PROXY_TYPE_HTTP,
                                             PROXY_TYPE_SOCKS})

Review comment:
       these should line up:
   ```suggestion
       ->withAllowableValues<std::string>({PROXY_TYPE_DIRECT,
                                           PROXY_TYPE_HTTP,
                                           PROXY_TYPE_SOCKS})
   ```

##########
File path: extensions/sftp/processors/ListSFTP.cpp
##########
@@ -59,73 +59,85 @@ namespace nifi {
 namespace minifi {
 namespace processors {
 
-core::Property ListSFTP::ListingStrategy(
-    core::PropertyBuilder::createProperty("Listing Strategy")->withDescription("Specify how to determine new/updated entities. See each strategy descriptions for detail.")
-        ->isRequired(true)
-        ->withAllowableValues<std::string>({LISTING_STRATEGY_TRACKING_TIMESTAMPS,
-                                            LISTING_STRATEGY_TRACKING_ENTITIES})
-        ->withDefaultValue(LISTING_STRATEGY_TRACKING_TIMESTAMPS)->build());
-core::Property ListSFTP::RemotePath(
-    core::PropertyBuilder::createProperty("Remote Path")->withDescription("The fully qualified filename on the remote system")
-        ->isRequired(false)->supportsExpressionLanguage(true)->build());
-core::Property ListSFTP::SearchRecursively(
-    core::PropertyBuilder::createProperty("Search Recursively")->withDescription("If true, will pull files from arbitrarily nested subdirectories; "
+core::Property ListSFTP::ListingStrategy(core::PropertyBuilder::createProperty("Listing Strategy")
+    ->withDescription("Specify how to determine new/updated entities. See each strategy descriptions for detail.")
+    ->isRequired(true)
+    ->withAllowableValues<std::string>({LISTING_STRATEGY_TRACKING_TIMESTAMPS, LISTING_STRATEGY_TRACKING_ENTITIES})
+    ->withDefaultValue(LISTING_STRATEGY_TRACKING_TIMESTAMPS)->build());
+
+core::Property ListSFTP::RemotePath(core::PropertyBuilder::createProperty("Remote Path")
+    ->withDescription("The fully qualified filename on the remote system")
+    ->isRequired(false)->supportsExpressionLanguage(true)->build());
+
+core::Property ListSFTP::SearchRecursively(core::PropertyBuilder::createProperty("Search Recursively")
+    ->withDescription("If true, will pull files from arbitrarily nested subdirectories; "
                                                                                  "otherwise, will not traverse subdirectories")

Review comment:
       this should be indented less:
   ```suggestion
       ->withDescription("If true, will pull files from arbitrarily nested subdirectories; "
                         "otherwise, will not traverse subdirectories")
   ```




-- 
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-unsubscribe@nifi.apache.org

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