Re: [PR] Working on api reduction [nifi-minifi-cpp]

["lordgamez (via GitHub)" <gi...@apache.org>]

lordgamez commented on code in PR #1930:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1930#discussion_r2158745582


##########
extension-utils/include/utils/ProcessorConfigUtils.h:
##########


Review Comment:
   Having `utils` and `extension-utils` still sounds like placeholder names and definitely should be renamed, especially as both of them have another `utils` subdirectory. If it's a more complicated change we can have it in a separate PR. As most of the files under `utils` and `extension-utils` not really general utilities but rather the core framework and extension framework implementation it could be something like `core-framework`-`extension-framework`, `core-impl`-`extension-impl`(as the counterpart of the minifi-api), or `core-defaults`-`extension-defaults` (for the default implementation), but we can workshop these a bit more.



##########
extensions/expression-language/ProcessContextExpr.h:
##########
@@ -34,25 +34,12 @@ namespace org::apache::nifi::minifi::core {
  */
 class ProcessContextExpr final : public core::ProcessContextImpl {
  public:
-  ProcessContextExpr(Processor& processor, controller::ControllerServiceProvider* controller_service_provider,
-                     const std::shared_ptr<core::Repository> &repo, const std::shared_ptr<core::Repository> &flow_repo,
-                     const std::shared_ptr<core::ContentRepository> &content_repo = core::repository::createFileSystemRepository())
-      : core::ProcessContextImpl(processor, controller_service_provider, repo, flow_repo, content_repo),
-        logger_(logging::LoggerFactory<ProcessContextExpr>::getLogger()) {
-  }
-
-  ProcessContextExpr(Processor& processor, controller::ControllerServiceProvider* controller_service_provider,
-                     const std::shared_ptr<core::Repository> &repo, const std::shared_ptr<core::Repository> &flow_repo, const std::shared_ptr<minifi::Configure> &configuration,
-                     const std::shared_ptr<core::ContentRepository> &content_repo = core::repository::createFileSystemRepository())
-      : core::ProcessContextImpl(processor, controller_service_provider, repo, flow_repo, configuration, content_repo),
-        logger_(logging::LoggerFactory<ProcessContextExpr>::getLogger()) {
-  }
-
+  using ProcessContextImpl::ProcessContextImpl;
   ~ProcessContextExpr() override = default;
 
   nonstd::expected<std::string, std::error_code> getProperty(std::string_view name, const FlowFile*) const override;
   nonstd::expected<std::string, std::error_code> getDynamicProperty(std::string_view name, const FlowFile*) const override;
-
+//

Review Comment:
   This can be removed



##########
utils/src/core/ProcessorImpl.cpp:
##########


Review Comment:
   Could we also rename the other *Impl class files to *Impl.h and *Impl.cpp for consistency? It's also easier to find the correct file when searching for it in the editor. If that's a larger change we can do it under a separate jira ticket in a follow up PR.



##########
libminifi/test/libtest/unit/TestBase.h:
##########
@@ -201,13 +201,56 @@ class TempDirectory {
   bool is_owner_;
 };
 
+template<typename T>
+class TypedProcessorWrapper {

Review Comment:
   Just curious, what was the reason for adding a wrapper for the tested processors? 



##########
libminifi/include/Port.h:
##########
@@ -44,4 +48,15 @@ class Port final : public ForwardingNode {
   PortType port_type_;
 };
 
+class Port : public core::Processor {

Review Comment:
   Why do we need a separate Port and PortImpl here?



##########
libminifi/include/Port.h:
##########
@@ -29,10 +30,13 @@ enum class PortType {
   OUTPUT
 };
 
-class Port final : public ForwardingNode {
+class Port;

Review Comment:
   Do we need a forward declaration here?



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