Centralize lets log prefix and colorize debug output#319
Open
Centralize lets log prefix and colorize debug output#319
Conversation
Reviewer's GuideCentralizes the Sequence diagram for centralized lets prefix and debug coloringsequenceDiagram
participant Caller as Caller
participant Logrus as Logrus_Logger
participant Formatter as Formatter
participant Color as FatihColor
Caller->>Logrus: Debugf("message")
activate Logrus
Logrus->>Formatter: Format(entry level=Debug message="message")
activate Formatter
Formatter->>Color: BlueString("lets:")
Color-->>Formatter: blueLetsPrefix
Formatter->>Formatter: writeData(entry.Data)
Formatter->>Formatter: formatMessage(entry)
Formatter->>Color: BlueString("message")
Color-->>Formatter: blueMessage
Formatter-->>Logrus: "blueLetsPrefix [data] blueMessage\n"
deactivate Formatter
Logrus-->>Caller: write to output
deactivate Logrus
Caller->>Logrus: Infof("message")
activate Logrus
Logrus->>Formatter: Format(entry level=Info message="message")
activate Formatter
Formatter->>Color: BlueString("lets:")
Color-->>Formatter: blueLetsPrefix
Formatter->>Formatter: writeData(entry.Data)
Formatter->>Formatter: formatMessage(entry)
Formatter-->>Logrus: "blueLetsPrefix [data] message\n"
deactivate Formatter
Logrus-->>Caller: write to output
deactivate Logrus
Sequence diagram for ExecLogger using shared formatter prefixsequenceDiagram
participant Caller as ExecutorCode
participant Exec as ExecLogger
participant Logrus as Logrus_Logger
participant Formatter as Formatter
Caller->>Exec: Child("build")
activate Exec
Exec->>Exec: create child with prefix="[build]"
Exec-->>Caller: childExec
deactivate Exec
Caller->>Exec: Info("running %s", cmd)
activate Exec
Exec->>Exec: if prefix != "" then prepend prefix
Exec->>Logrus: Logf(Info, "[build] running %s", cmd)
deactivate Exec
activate Logrus
Logrus->>Formatter: Format(entry message="[build] running make")
activate Formatter
Formatter->>Formatter: writeData(entry.Data)
Formatter->>Formatter: formatMessage(entry level=Info)
Formatter-->>Logrus: "lets: [build] running make\n"
deactivate Formatter
Logrus-->>Caller: write to output
deactivate Logrus
Updated class diagram for logging formatter and ExecLoggerclassDiagram
class Formatter {
+Format(entry *log.Entry) []byte
+formatMessage(entry *log.Entry) string
+writeData(fields log.Fields) string
}
class ExecLogger {
-log *log.Logger
-name string
-prefix string
-cache map[string]*ExecLogger
+NewExecLogger() *ExecLogger
+Child(name string) *ExecLogger
+Info(format string, a ...any)
+Debug(format string, a ...any)
}
class Logrus_Logger {
+Logf(level log.Level, format string, a ...any)
+StandardLogger() *Logrus_Logger
}
class Color {
+BlueString(format string, a ...any) string
+GreenString(format string, a ...any) string
+YellowString(format string, a ...any) string
+RedString(format string, a ...any) string
}
ExecLogger --> Logrus_Logger : uses
Formatter --> Color : uses
Logrus_Logger --> Formatter : uses as Formatter
ExecLogger --> ExecLogger : cache children by name
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
TestFormatterColorsDebugMessagesmutates the globalcolor.NoColorflag and could race with other tests that rely onfatih/color; consider scoping this via a helper that serializes such tests or avoiding global mutation altogether. - In
formatMessage, onlylog.DebugLevelmessages are colorized; if you intend consistent treatment for other verbose levels (e.g.,TraceLevel), consider broadening the condition or documenting that only debug is special-cased.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `TestFormatterColorsDebugMessages` mutates the global `color.NoColor` flag and could race with other tests that rely on `fatih/color`; consider scoping this via a helper that serializes such tests or avoiding global mutation altogether.
- In `formatMessage`, only `log.DebugLevel` messages are colorized; if you intend consistent treatment for other verbose levels (e.g., `TraceLevel`), consider broadening the condition or documenting that only debug is special-cased.
## Individual Comments
### Comment 1
<location path="internal/logging/log_test.go" line_range="42-51" />
<code_context>
})
}
+
+func TestFormatterColorsDebugMessages(t *testing.T) {
+ prevNoColor := color.NoColor
+ color.NoColor = false
+ defer func() {
+ color.NoColor = prevNoColor
+ }()
+
+ line, err := (&Formatter{}).Format(&log.Entry{
+ Level: log.DebugLevel,
+ Message: "debug message",
+ })
+ if err != nil {
+ t.Fatalf("Format() error = %v", err)
+ }
+
+ expected := color.BlueString("lets:") + " " + color.BlueString("debug message") + "\n"
+ if string(line) != expected {
+ t.Fatalf("unexpected debug line: %q", string(line))
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests to cover non-debug levels and messages with structured fields to fully exercise the new formatter behavior.
Since we only test the debug case without structured fields, please add table-driven tests that cover: (1) info/warn/error levels (non-colorized message), (2) entries with `Data` set (one or more fields) to verify spacing and ordering around `lets:`, fields, and the message, and (3) an empty `Data` case to ensure no extra spaces. This will better validate the new centralized prefixing/spacing logic.
Suggested implementation:
```golang
func TestFormatterColorsDebugMessages(t *testing.T) {
prevNoColor := color.NoColor
color.NoColor = false
defer func() {
color.NoColor = prevNoColor
}()
line, err := (&Formatter{}).Format(&log.Entry{
Level: log.DebugLevel,
Message: "debug message",
})
if err != nil {
t.Fatalf("Format() error = %v", err)
}
expected := color.BlueString("lets:") + " " + color.BlueString("debug message") + "\n"
if string(line) != expected {
t.Fatalf("unexpected debug line: %q", string(line))
}
}
func TestFormatterFormatsLevelsAndFields(t *testing.T) {
tests := []struct {
name string
entry *log.Entry
}{
{
name: "info_no_data",
entry: &log.Entry{
Level: log.InfoLevel,
Message: "info message",
Data: log.Fields{},
},
},
{
name: "warn_with_single_field",
entry: &log.Entry{
Level: log.WarnLevel,
Message: "warn message",
Data: log.Fields{
"foo": "bar",
},
},
},
{
name: "error_with_multiple_fields",
entry: &log.Entry{
Level: log.ErrorLevel,
Message: "error message",
Data: log.Fields{
"alpha": "one",
"beta": "two",
},
},
},
}
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
lineBytes, err := (&Formatter{}).Format(tt.entry)
if err != nil {
t.Fatalf("Format() error = %v", err)
}
line := string(lineBytes)
// All levels should be prefixed consistently.
if !strings.HasPrefix(line, "lets: ") {
t.Fatalf("line does not start with expected prefix: %q", line)
}
// Non-debug messages should not be colorized by the formatter under default settings.
if strings.Contains(line, "\x1b[") {
t.Fatalf("expected non-colorized output for non-debug levels, got: %q", line)
}
// Empty Data: ensure there are no extra spaces (only one space after the prefix).
if len(tt.entry.Data) == 0 {
expected := "lets: " + tt.entry.Message + "\n"
if line != expected {
t.Fatalf("unexpected formatted line for empty Data.\nexpected: %q\ngot: %q", expected, line)
}
return
}
// With Data: verify ordering and spacing around prefix, fields, and message.
prefixIdx := strings.Index(line, "lets:")
if prefixIdx != 0 {
t.Fatalf("prefix not at beginning of line: %q", line)
}
msgIdx := strings.LastIndex(line, tt.entry.Message)
if msgIdx == -1 {
t.Fatalf("message %q not found in line: %q", tt.entry.Message, line)
}
if msgIdx <= prefixIdx {
t.Fatalf("message appears before prefix in line: %q", line)
}
// Ensure no double spaces in the formatted output.
if strings.Contains(line, " ") {
t.Fatalf("unexpected double spaces in line: %q", line)
}
// Each field should appear between the prefix and the message.
for key, rawVal := range tt.entry.Data {
val, ok := rawVal.(string)
if !ok {
t.Fatalf("test setup error: expected string value for key %q, got %T", key, rawVal)
}
fieldStr := key + "=" + val
fieldIdx := strings.Index(line, fieldStr)
if fieldIdx == -1 {
t.Fatalf("field %q not found in line: %q", fieldStr, line)
}
if !(fieldIdx > prefixIdx && fieldIdx < msgIdx) {
t.Fatalf("field %q not positioned between prefix and message in line: %q", fieldStr, line)
}
}
})
}
}
```
To compile these tests, update the imports in `internal/logging/log_test.go`:
1. Add the `strings` package to the import list:
- `import "strings"`
Make sure no existing imports are removed, and keep the import block sorted/grouped according to your current conventions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+42
to
+51
| func TestFormatterColorsDebugMessages(t *testing.T) { | ||
| prevNoColor := color.NoColor | ||
| color.NoColor = false | ||
| defer func() { | ||
| color.NoColor = prevNoColor | ||
| }() | ||
|
|
||
| line, err := (&Formatter{}).Format(&log.Entry{ | ||
| Level: log.DebugLevel, | ||
| Message: "debug message", |
There was a problem hiding this comment.
suggestion (testing): Add tests to cover non-debug levels and messages with structured fields to fully exercise the new formatter behavior.
Since we only test the debug case without structured fields, please add table-driven tests that cover: (1) info/warn/error levels (non-colorized message), (2) entries with Data set (one or more fields) to verify spacing and ordering around lets:, fields, and the message, and (3) an empty Data case to ensure no extra spaces. This will better validate the new centralized prefixing/spacing logic.
Suggested implementation:
func TestFormatterColorsDebugMessages(t *testing.T) {
prevNoColor := color.NoColor
color.NoColor = false
defer func() {
color.NoColor = prevNoColor
}()
line, err := (&Formatter{}).Format(&log.Entry{
Level: log.DebugLevel,
Message: "debug message",
})
if err != nil {
t.Fatalf("Format() error = %v", err)
}
expected := color.BlueString("lets:") + " " + color.BlueString("debug message") + "\n"
if string(line) != expected {
t.Fatalf("unexpected debug line: %q", string(line))
}
}
func TestFormatterFormatsLevelsAndFields(t *testing.T) {
tests := []struct {
name string
entry *log.Entry
}{
{
name: "info_no_data",
entry: &log.Entry{
Level: log.InfoLevel,
Message: "info message",
Data: log.Fields{},
},
},
{
name: "warn_with_single_field",
entry: &log.Entry{
Level: log.WarnLevel,
Message: "warn message",
Data: log.Fields{
"foo": "bar",
},
},
},
{
name: "error_with_multiple_fields",
entry: &log.Entry{
Level: log.ErrorLevel,
Message: "error message",
Data: log.Fields{
"alpha": "one",
"beta": "two",
},
},
},
}
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
lineBytes, err := (&Formatter{}).Format(tt.entry)
if err != nil {
t.Fatalf("Format() error = %v", err)
}
line := string(lineBytes)
// All levels should be prefixed consistently.
if !strings.HasPrefix(line, "lets: ") {
t.Fatalf("line does not start with expected prefix: %q", line)
}
// Non-debug messages should not be colorized by the formatter under default settings.
if strings.Contains(line, "\x1b[") {
t.Fatalf("expected non-colorized output for non-debug levels, got: %q", line)
}
// Empty Data: ensure there are no extra spaces (only one space after the prefix).
if len(tt.entry.Data) == 0 {
expected := "lets: " + tt.entry.Message + "\n"
if line != expected {
t.Fatalf("unexpected formatted line for empty Data.\nexpected: %q\ngot: %q", expected, line)
}
return
}
// With Data: verify ordering and spacing around prefix, fields, and message.
prefixIdx := strings.Index(line, "lets:")
if prefixIdx != 0 {
t.Fatalf("prefix not at beginning of line: %q", line)
}
msgIdx := strings.LastIndex(line, tt.entry.Message)
if msgIdx == -1 {
t.Fatalf("message %q not found in line: %q", tt.entry.Message, line)
}
if msgIdx <= prefixIdx {
t.Fatalf("message appears before prefix in line: %q", line)
}
// Ensure no double spaces in the formatted output.
if strings.Contains(line, " ") {
t.Fatalf("unexpected double spaces in line: %q", line)
}
// Each field should appear between the prefix and the message.
for key, rawVal := range tt.entry.Data {
val, ok := rawVal.(string)
if !ok {
t.Fatalf("test setup error: expected string value for key %q, got %T", key, rawVal)
}
fieldStr := key + "=" + val
fieldIdx := strings.Index(line, fieldStr)
if fieldIdx == -1 {
t.Fatalf("field %q not found in line: %q", fieldStr, line)
}
if !(fieldIdx > prefixIdx && fieldIdx < msgIdx) {
t.Fatalf("field %q not positioned between prefix and message in line: %q", fieldStr, line)
}
}
})
}
}To compile these tests, update the imports in internal/logging/log_test.go:
- Add the
stringspackage to the import list:import "strings"
Make sure no existing imports are removed, and keep the import block sorted/grouped according to your current conventions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lets:prefixes into the shared formatter and adjust call sites/formatter tests accordinglylets:Testing
Summary by Sourcery
Centralize the
lets:log prefix in the shared log formatter, simplify call sites and exec logger prefixes, and ensure debug output is consistently colored while updating documentation accordingly.Enhancements:
lets:prefix and applies blue coloring to debug-level messages.lets:prefix while keeping per-command prefixes colorized without duplication.lets:prefixes in favor of the centralized formatter.Documentation:
Tests: