fgerlits commented on code in PR #1670: URL: https://github.com/apache/nifi-minifi-cpp/pull/1670#discussion_r1347541034
########## extensions/lua/LuaLogger.h: ########## @@ -0,0 +1,38 @@ +/** + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include <string> +#include <memory> + +#include "sol/sol.hpp" +#include "core/StateManager.h" + +namespace org::apache::nifi::minifi::extensions::lua { + +class LuaLogger { + public: + explicit LuaLogger(core::logging::Logger* logger) : logger_(logger) {} Review Comment: we could make the argument `gsl::not_null`, too, to catch some errors at compile time instead of runtime ########## extensions/librdkafka/ConsumeKafka.cpp: ########## @@ -133,7 +133,7 @@ void ConsumeKafka::create_topic_partition_list() { // This might happen until the cross-overship between processors and connections are settled rd_kafka_resp_err_t subscribe_response = rd_kafka_subscribe(consumer_.get(), kf_topic_partition_list_.get()); if (RD_KAFKA_RESP_ERR_NO_ERROR != subscribe_response) { - logger_->log_error("rd_kafka_subscribe error %d: %s", subscribe_response, rd_kafka_err2str(subscribe_response)); + logger_->log_error("rd_kafka_subscribe error {}: {}", static_cast<int>(subscribe_response), rd_kafka_err2str(subscribe_response)); Review Comment: can we use `magic_enum::enum_underlying(subscribe_response)` here? (also for the other `static_cast<int>`s in this file) ########## extensions/mqtt/processors/ConsumeMQTT.cpp: ########## @@ -304,7 +304,7 @@ void ConsumeMQTT::checkBrokerLimitsImpl() { throw Exception(PROCESS_SCHEDULE_EXCEPTION, os.str()); } } else { - logger_->log_warn("Shared topic feature with topic \"%s\" might not be supported by broker on MQTT 3.x"); + logger_->log_warn("Shared topic feature with topic \"{}\" might not be supported by broker on MQTT 3.x", topic_); Review Comment: :beers: ########## extensions/lua/LuaLogger.h: ########## @@ -0,0 +1,38 @@ +/** + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include <string> +#include <memory> + +#include "sol/sol.hpp" +#include "core/StateManager.h" + +namespace org::apache::nifi::minifi::extensions::lua { + +class LuaLogger { + public: + explicit LuaLogger(core::logging::Logger* logger) : logger_(logger) {} + + void log_info(std::string log_message) { logger_->log_info("{}", log_message); } Review Comment: the parameter could be `string_view` instead of `string` ########## extensions/librdkafka/ConsumeKafka.cpp: ########## @@ -297,9 +297,9 @@ std::vector<std::string> ConsumeKafka::get_matching_headers(const rd_kafka_messa break; } if (size < 200) { - logger_->log_debug("%.*s", static_cast<int>(size), value); + logger_->log_debug("{:.{}}", value, static_cast<int>(size)); Review Comment: I think we can remove the `static_cast<int>` ########## extensions/standard-processors/processors/GetTCP.cpp: ########## @@ -266,7 +266,7 @@ asio::awaitable<void> GetTCP::TcpClient::doReceiveFrom(const utils::net::Connect continue; } } - logger_->log_error("Error connecting to %s:%s due to %s", connection_id.getHostname().data(), connection_id.getService().data(), last_error.message()); + logger_->log_error("Error connecting to {}:{} due to {}", connection_id.getHostname().data(), connection_id.getService().data(), last_error.message()); Review Comment: these `.data()`s should be removed, because they are both unnecessary (now), and dangerous if the `string_view` is not null-terminated ########## extensions/sftp/processors/ListSFTP.cpp: ########## @@ -539,7 +537,7 @@ void ListSFTP::listByTrackingTimestamps( } /* If the latest timestamp is not old enough, we wait another cycle */ if (latest_listed_entry_timestamp_this_cycle && minimum_reliable_timestamp < latest_listed_entry_timestamp_this_cycle) { - logger_->log_debug("Skipping files with latest timestamp because their modification date is not smaller than the minimum reliable timestamp: %lu ms >= %lu ms", + logger_->log_debug("Skipping files with latest timestamp because their modification date is not smaller than the minimum reliable timestamp: {} ms >= {} ms", toUnixTime(latest_listed_entry_timestamp_this_cycle), toUnixTime(minimum_reliable_timestamp)); Review Comment: I think fmt can handle `time_point`s; could/should we get rid of the `toUnixTime`s here? ########## extensions/standard-processors/controllers/PersistentMapStateStorage.cpp: ########## @@ -132,10 +132,10 @@ void PersistentMapStateStorage::onEnable() { } const auto always_persist = getProperty<bool>(AlwaysPersist).value_or(false); - logger_->log_info("Always Persist property: %s", always_persist ? "true" : "false"); + logger_->log_info("Always Persist property: {}", always_persist ? "true" : "false"); Review Comment: we can get rid of them later separately if you don't want to further increase the size of this PR, but I think these `? "true" : "false"`es are no longer 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
