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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
