-
Notifications
You must be signed in to change notification settings - Fork 440
fix: Don't log if --json or --quiet
#4202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 45 commits
d9ac264
243d928
bb89645
58dc097
9ca3a20
22a1177
fe59cb8
5515fb1
f529c23
dee6f12
4f3db35
e62c40e
5596d03
91c65a8
2f7c9ae
808be20
1e752c7
ce45264
ee538ba
63ed7bd
3fc7d69
cfeadc9
b6736ee
fafe861
b3b5110
139beec
7eeed5a
30de13b
b7f20cf
42aef3d
5e72263
a8753d3
0fce75f
1394350
a5620e8
b8b7c12
b962248
4a78575
38c3bc9
7c7d66a
17e9a06
a98b70e
e495113
007e13a
9d8b7bd
240d53d
daeb4ee
3e21106
da56561
2146be8
3b3ebae
ace8813
395cbcb
05c46ac
ba8cfd5
fa0f430
fd5a8e8
855eb58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,18 @@ namespace mamba::logging | |
| { out.flush() }; | ||
| }; | ||
|
|
||
| /** Transforms a source location into a human-readable string ready for logging purpose. */ | ||
| inline auto as_log(const std::source_location& location) -> std::string | ||
| { | ||
| return fmt::format( | ||
| "{}:{}:{} {}", | ||
| location.file_name(), | ||
| location.line(), | ||
| location.column(), | ||
| location.function_name() | ||
| ); | ||
| } | ||
|
|
||
| namespace details | ||
| { | ||
| inline auto queue_push(std::deque<LogRecord>& queue, size_t max_elements, LogRecord record) | ||
|
|
@@ -37,6 +49,21 @@ namespace mamba::logging | |
| } | ||
| } | ||
|
|
||
| inline auto should_be_ignored(const LogRecord& record, log_level current_level) -> bool | ||
| { | ||
| switch (current_level) | ||
| { | ||
| case log_level::off: | ||
| return true; | ||
|
|
||
| case log_level::all: | ||
| return false; | ||
|
|
||
| default: | ||
| return current_level > record.level; | ||
| } | ||
| } | ||
|
|
||
| /** Backtrace feature implementation in it's most basic form. | ||
|
|
||
| This is the simplest implementation for a backtrace feature | ||
|
|
@@ -67,14 +94,15 @@ namespace mamba::logging | |
| */ | ||
| auto push_if_enabled(LogRecord& record) -> bool | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When this function returns if (not backtrace.push_if_enabled(std::move(record)))
{
something_else(std::move(record)); // looks like UB
}My goal here is to have this algorithm written only once, as having the same logic in both place lead to some annoying errors while developing. But I also agree it's quite weird api. I'll try to find another way to achieve that goal with a nicer api. I was thinking maybe something in that direction: // free function:
auto push_to_backtrace_or(Record&& record, BasicBacktrace& backtrace, std::invocable<Record&&> auto or_func);
// in call site:
push_to_backtrace_or(std::move(record), backtrace, [&](LogRecord&& record){
// do the work if not pushed to backtrace
something_else(std::move(record));
});The passing of the same object to the lambda is a bit weird but maybe less weird than this current api?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking of having |
||
| { | ||
| if (not is_enabled()) | ||
| if (is_enabled()) | ||
| { | ||
| queue_push(backtrace, backtrace_max, std::move(record)); | ||
| return true; | ||
| } | ||
| else | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| queue_push(backtrace, backtrace_max, std::move(record)); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /** Changes the number of log records kept in the backtrace history. | ||
|
|
@@ -138,17 +166,6 @@ namespace mamba::logging | |
| } | ||
| }; | ||
|
|
||
| inline auto as_log(const std::source_location& location) -> std::string | ||
| { | ||
| return fmt::format( | ||
| "{}:{}:{} {}", | ||
| location.file_name(), | ||
| location.line(), | ||
| location.column(), | ||
| location.function_name() | ||
| ); | ||
| } | ||
|
|
||
| struct log_to_stream_options | ||
| { | ||
| bool with_location = false; | ||
|
|
@@ -158,9 +175,8 @@ namespace mamba::logging | |
| log_to_stream(OutputStream auto& out, const LogRecord& record, log_to_stream_options options = {}) | ||
| -> OutputStream auto& | ||
| { | ||
| auto location_str = options.with_location | ||
| ? fmt::format(" ({})", details::as_log(record.location)) | ||
| : std::string{}; | ||
| auto location_str = options.with_location ? fmt::format(" ({})", as_log(record.location)) | ||
| : std::string{}; | ||
|
|
||
| out << fmt::format( | ||
| "\n{} {}{} : {}", | ||
|
|
@@ -234,7 +250,13 @@ namespace mamba::logging | |
|
|
||
| auto enable_backtrace(size_t record_buffer_size) -> void; | ||
| auto disable_backtrace() -> void; | ||
| auto log_backtrace() -> void; | ||
|
|
||
| /** @see `AnyLogHandler::log_backtrace()` | ||
|
|
||
| @param filter_current_level If `true`, will filter out log records that would not | ||
| pass the log level filter if they were emitted right now. | ||
| */ | ||
| auto log_backtrace(bool filter_current_level = false) -> void; | ||
| auto log_backtrace_no_guards() -> void; | ||
|
|
||
| auto flush(std::optional<log_source> source = {}) -> void; | ||
|
|
@@ -245,12 +267,14 @@ namespace mamba::logging | |
| //////////////////////////////////////////// | ||
| // History api - thread-safe | ||
|
|
||
| /** @returns A copy of the current log record history. | ||
| /** Captures the currently stored sequence of `LogRecord`. | ||
| @param `and_clear` Will also clear the existing history if `true`. | ||
| @returns A copy of the current log record history. | ||
| The value should be considered immediately obsolete | ||
| as new log records could be pushed concurrently. | ||
| The returned history will be empty if `is_started()` == false. | ||
| */ | ||
| auto capture_history() const -> std::vector<LogRecord>; | ||
| auto capture_history(bool and_clear = false) const -> std::vector<LogRecord>; | ||
|
|
||
| /** Clears the internal history. | ||
|
|
||
|
|
@@ -343,7 +367,13 @@ namespace mamba::logging | |
|
|
||
| auto enable_backtrace(size_t record_buffer_size) -> void; | ||
| auto disable_backtrace() -> void; | ||
| auto log_backtrace() -> void; | ||
|
|
||
| /** @see `AnyLogHandler::log_backtrace()` | ||
|
|
||
| @param filter_current_level If `true`, will filter out log records that would not | ||
| pass the log level filter if they were emitted right now. | ||
| */ | ||
| auto log_backtrace(bool filter_current_level = false) -> void; | ||
| auto log_backtrace_no_guards() -> void; | ||
|
|
||
| auto flush(std::optional<log_source> source = {}) -> void; | ||
|
|
@@ -431,7 +461,7 @@ namespace mamba::logging | |
| inline auto LogHandler_History::log(LogRecord record) -> void | ||
| { | ||
| assert(pimpl); | ||
| if (pimpl->current_log_level < record.level) | ||
| if (details::should_be_ignored(record, pimpl->current_log_level)) | ||
| { | ||
| return; | ||
| } | ||
|
|
@@ -455,12 +485,16 @@ namespace mamba::logging | |
| pimpl->data->backtrace.disable(); | ||
| } | ||
|
|
||
| inline auto LogHandler_History::log_backtrace() -> void | ||
| inline auto LogHandler_History::log_backtrace(bool filter_current_level) -> void | ||
| { | ||
| assert(pimpl); | ||
| auto synched_data = pimpl->data.synchronize(); | ||
| for (auto& log : synched_data->backtrace) | ||
| { | ||
| if (filter_current_level and details::should_be_ignored(log, pimpl->current_log_level)) | ||
| { | ||
| continue; | ||
| } | ||
| details::queue_push(synched_data->history, options.max_records_count, std::move(log)); | ||
| } | ||
|
|
||
|
|
@@ -470,7 +504,8 @@ namespace mamba::logging | |
| inline auto LogHandler_History::log_backtrace_no_guards() -> void | ||
| { | ||
| assert(pimpl); | ||
| log_backtrace(); // Similar in this context | ||
| log_backtrace(true); // Similar in this context but we want to filter out logs that would | ||
| // not pass the level filter now | ||
| } | ||
|
|
||
| inline auto LogHandler_History::flush(std::optional<log_source>) -> void | ||
|
|
@@ -485,12 +520,25 @@ namespace mamba::logging | |
| // nothing to do, we keep history, there is no flush | ||
| } | ||
|
|
||
| inline auto LogHandler_History::capture_history() const -> std::vector<LogRecord> | ||
| inline auto LogHandler_History::capture_history(bool and_clear) const -> std::vector<LogRecord> | ||
| { | ||
| if (pimpl) | ||
| { | ||
| auto synched_data = pimpl->data.synchronize(); | ||
| return std::vector<LogRecord>(synched_data->history.begin(), synched_data->history.end()); | ||
| if (and_clear) | ||
| { | ||
| std::vector<LogRecord> result(synched_data->history.size()); | ||
| std::ranges::move(synched_data->history, result.begin()); | ||
| synched_data->history.clear(); | ||
|
jjerphan marked this conversation as resolved.
|
||
| return result; | ||
| } | ||
| else | ||
| { | ||
| return std::vector<LogRecord>( | ||
| synched_data->history.begin(), | ||
| synched_data->history.end() | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return {}; | ||
|
|
@@ -504,7 +552,7 @@ namespace mamba::logging | |
| } | ||
| } | ||
|
|
||
| auto LogHandler_History::is_started() const -> bool | ||
| inline auto LogHandler_History::is_started() const -> bool | ||
| { | ||
| return pimpl != nullptr; | ||
| } | ||
|
|
@@ -588,7 +636,7 @@ namespace mamba::logging | |
| assert(out); | ||
| assert(pimpl); | ||
|
|
||
| if (pimpl->current_log_level > record.level) | ||
| if (details::should_be_ignored(record, pimpl->current_log_level)) | ||
| { | ||
| return; | ||
| } | ||
|
|
@@ -625,14 +673,19 @@ namespace mamba::logging | |
| } | ||
|
|
||
| template <OutputStream T> | ||
| inline auto LogHandler_Stream<T>::log_backtrace() -> void | ||
| inline auto LogHandler_Stream<T>::log_backtrace(bool filter_current_level) -> void | ||
| { | ||
| assert(out); | ||
| assert(pimpl); | ||
|
|
||
| auto synched_backtrace = pimpl->backtrace.synchronize(); | ||
| for (auto& log_record : *synched_backtrace) | ||
| { | ||
| if (filter_current_level | ||
| and details::should_be_ignored(log_record, pimpl->current_log_level)) | ||
| { | ||
| continue; | ||
| } | ||
| details::log_to_stream(*out, log_record, { .with_location = pimpl->log_location }); | ||
| } | ||
| synched_backtrace->clear(); | ||
|
|
@@ -644,7 +697,8 @@ namespace mamba::logging | |
| assert(out); | ||
| assert(pimpl); | ||
|
|
||
| log_backtrace(); // Similar in this context | ||
| log_backtrace(true); // Similar in this context but we want to filter out logs that would | ||
| // not pass the level filter now | ||
| } | ||
|
|
||
| template <OutputStream T> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,6 +129,11 @@ namespace mamba | |
| ///< object to mark it as a success will be | ||
| ///< done once the other changes are | ||
| ///< applied. | ||
|
|
||
| /// Construct a json edit based on the members of a json object by using | ||
| /// each member as an assignation into the json object that will be edited | ||
| /// by this resulgint json edit. | ||
| static auto from_json_object_members(nlohmann::json object) -> JSONEdit; | ||
| }; | ||
|
|
||
| class Console | ||
|
|
@@ -183,11 +188,31 @@ namespace mamba | |
| */ | ||
| void set_json_output(JSONEdit edit); | ||
|
|
||
| /** Assigns the provided json value to the associated location in the json output object. | ||
| This overload is a shorter version for the 1 location 1 value assignation case | ||
| which is quite common. | ||
| */ | ||
| template <class T> | ||
| requires requires(const T& value) { nlohmann::json{ value }; } | ||
| void set_json_output(nlohmann::json::json_pointer location, T&& value) | ||
| { | ||
| // clang-format off | ||
| set_json_output({ | ||
| .to_assign{ | ||
| { std::move(location), std::forward<T>(value) } | ||
| } | ||
| }); | ||
| // clang-format on | ||
| } | ||
|
|
||
| /** If json output was requested, calling this before destroying a `Console` instance will | ||
| not lead to a json output. | ||
| */ | ||
| void cancel_json_print(); | ||
|
|
||
| // FIXME: documentation | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you mean to introduce a small docstring?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to add the docstring yes, will add. |
||
| static void setup_log_handling_for_json(); | ||
|
|
||
| static void print_buffer(std::ostream& ostream); | ||
|
|
||
| const Context& context() const; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.