-
-
Notifications
You must be signed in to change notification settings - Fork 3
Use Slog #32
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces the standard library’s log usage with structured slog logging, updates the HTTP and exec components to accept *slog.Logger, and enhances the CLI to configure slog output format, level, and source.
- Migrate imports and calls from
logtoslogand inject*slog.Loggerinto servers and exec logic - Update tests to use
slog.Debuginstead oflog.Printf - Enhance
getLoggerto accept flags for JSON/text output, log level, and source location
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server.go | Swapped log for slog, changed ErrorLog/InfoLog types, but some handlers still call global slog.Info. |
| exec_test.go | Swapped log.Printf to slog.Debug in tests. |
| exec.go | Replaced Logger interface with *slog.Logger; updated InfoLogf/InfoLogln methods. |
| cmd/hookah/main.go | Expanded getLogger to handle JSON, levels, and source; call slog.SetDefault; updated startup logs. |
Comments suppressed due to low confidence (1)
exec_test.go:50
- [nitpick] Using the same key and message (
"scripts") is confusing; consider a clearer message likeslog.Debug("found scripts", "scripts", scripts).
slog.Debug("scripts", "scripts", scripts)
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| log.Println(ghDelivery, err) | ||
| slog.Info("request error", "delivery", ghDelivery, "error", err) |
Copilot
AI
Jun 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses the global logger and logs at Info level—errors should use the injected h.ErrorLog at Error level (e.g. h.ErrorLog.Error(...)) to respect server configuration.
| slog.Info("request error", "delivery", ghDelivery, "error", err) | |
| if h.ErrorLog != nil { | |
| h.ErrorLog.Error("request error", "delivery", ghDelivery, "error", err) | |
| } |
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusBadRequest) | ||
| log.Println(ghDelivery, err) | ||
| slog.Info("json decode error", "delivery", ghDelivery, "error", err) |
Copilot
AI
Jun 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace this global slog.Info call with the instance’s h.ErrorLog.Error(...) to log decoding failures correctly and honor configured log destinations and levels.
| slog.Info("json decode error", "delivery", ghDelivery, "error", err) | |
| if h.ErrorLog != nil { | |
| h.ErrorLog.Error("json decode error", | |
| "delivery", ghDelivery, | |
| "error", err) | |
| } |
| slog.Error("unknown log level", "level", logLevel) | ||
| os.Exit(3) |
Copilot
AI
Jun 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Calling os.Exit and logging with the global logger here hinders testability and errors handling. Instead, return an error for unknown levels and let the caller decide to exit.
| slog.Error("unknown log level", "level", logLevel) | |
| os.Exit(3) | |
| return nil, fmt.Errorf("unknown log level: %s", logLevel) |
|
These are breaking changes… I need to consider these |
No description provided.