Conversation
noopur23
force-pushed
the
color-logging
branch
from
fa6daef to
ffbae22
Compare
g11tech
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds color support to both console and file logging in the Zeam project. The changes include colored output for different log levels (error, warn, info, debug), timestamps, and scopes, with an option to disable colors in file logs for compatibility with non-color-supporting editors.
- Add color constants and formatting to the logging system with ANSI escape codes
- Introduce a
monocolorFileoption to control colored output in log files - Add a new
getTestLogger()function for test environments with debug-level logging and no file output
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkgs/utils/src/log.zig | Adds color support with ANSI escape codes, implements file color toggling, and introduces test logger functionality |
| pkgs/state-transition/src/mock.zig | Updates to use the new test logger instead of regular logger |
| pkgs/state-transition/src/lib.zig | Updates test functions to use the new test logger |
| pkgs/node/src/forkchoice.zig | Updates test to use the new test logger |
| pkgs/node/src/chain.zig | Updates test to use the new test logger |
| pkgs/cli/src/main.zig | Adds CLI option for monochrome file logging and ensures log directory creation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| .fileActiveLevel = fileParams.?.fileBehaviour.fileActiveLevel, | ||
| .file = fileParams.?.file.?, | ||
| } | ||
| FileLogParams{ .fileActiveLevel = fileParams.?.fileBehaviour.fileActiveLevel, .file = fileParams.?.file.?, .monocolorFile = fileParams.?.fileBehaviour.monocolorFile } |
There was a problem hiding this comment.
[nitpick] This struct initialization is very long and hard to read on a single line. Consider breaking it into multiple lines for better readability.
| FileLogParams{ .fileActiveLevel = fileParams.?.fileBehaviour.fileActiveLevel, .file = fileParams.?.file.?, .monocolorFile = fileParams.?.fileBehaviour.monocolorFile } | |
| FileLogParams{ | |
| .fileActiveLevel = fileParams.?.fileBehaviour.fileActiveLevel, | |
| .file = fileParams.?.file.?, | |
| .monocolorFile = fileParams.?.fileBehaviour.monocolorFile, | |
| } |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| var print_str = std.fmt.bufPrint( | ||
| buf[0..], | ||
| "{s} {s}" ++ fmt ++ "\n", | ||
| .{ timestamp_str, prefix } ++ args, | ||
| "{s}{s}{s} {s}[{s}]{s} {s}{s}{s}" ++ fmt ++ "\n", | ||
| .{ timestamp_color, timestamp_str, reset_color, level_color, comptime level.asText(), reset_color, scope_color, scope_prefix, reset_color } ++ args, | ||
| ) catch return; |
There was a problem hiding this comment.
The format string is complex and difficult to read with 9 format specifiers. Consider breaking this into separate formatting steps or using a helper function to construct the colored message parts.
| print_str = std.fmt.bufPrint( | ||
| buf[0..], | ||
| "{s} {s}" ++ fmt ++ "\n", | ||
| .{ timestamp_str, prefix } ++ args, | ||
| ) catch return; |
There was a problem hiding this comment.
The variable prefix is used but not defined in the current scope. This should likely be the original format that was replaced by the colored version above.
|
|
||
| nosuspend fileLogParams.?.file.writeAll(print_str) catch |err| { | ||
| stderr.print("{s} [ERROR] {s}: Failed to write to log file : {any}\n", .{ timestamp_str, scope_prefix, err }) catch {}; | ||
| stderr.print("{s}{s}{s} {s}[ERROR]{s} {s}{s}{s}Failed to write to log file: {any}\n", .{ timestamp_color, timestamp_str, reset_color, Colors.err, reset_color, scope_color, scope_prefix, reset_color, err }) catch {}; |
There was a problem hiding this comment.
This error message format string is also overly complex with 9 format specifiers. Consider using a helper function for consistent error message formatting.
| @@ -265,6 +270,10 @@ pub fn getLogger(activeLevel: ?std.log.Level, fileBehaviourParams: ?FileBehaviou | |||
| return ZeamLogger.init(.default, activeLevel orelse std.log.default_level, fileBehaviourParams); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The getTestLogger function lacks documentation explaining its purpose and when it should be used instead of getLogger.
| /// Returns a ZeamLogger instance configured for use in test environments. | |
| /// Unlike `getLogger`, this logger always uses the default log level and does not write to any file. | |
| /// Use this function when you need a logger for tests and do not require file logging or custom log levels. |
closes #140