Skip to content

Use Slog - #32

Open
donatj wants to merge 2 commits into
masterfrom
slog
Open

Use Slog#32
donatj wants to merge 2 commits into
masterfrom
slog

Conversation

@donatj

@donatj donatj commented Jun 24, 2025

Copy link
Copy Markdown
Owner

No description provided.

@donatj
donatj requested a review from Copilot June 24, 2025 17:20

Copilot AI left a comment

Copy link
Copy Markdown

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 log to slog and inject *slog.Logger into servers and exec logic
  • Update tests to use slog.Debug instead of log.Printf
  • Enhance getLogger to 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 like slog.Debug("found scripts", "scripts", scripts).
	slog.Debug("scripts", "scripts", scripts)

Comment thread server.go
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

Copy link

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.

Suggested change
slog.Info("request error", "delivery", ghDelivery, "error", err)
if h.ErrorLog != nil {
h.ErrorLog.Error("request error", "delivery", ghDelivery, "error", err)
}

Copilot uses AI. Check for mistakes.
Comment thread server.go
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

Copy link

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.

Suggested change
slog.Info("json decode error", "delivery", ghDelivery, "error", err)
if h.ErrorLog != nil {
h.ErrorLog.Error("json decode error",
"delivery", ghDelivery,
"error", err)
}

Copilot uses AI. Check for mistakes.
Comment thread cmd/hookah/main.go
Comment on lines +99 to +100
slog.Error("unknown log level", "level", logLevel)
os.Exit(3)

Copilot AI Jun 24, 2025

Copy link

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.

Suggested change
slog.Error("unknown log level", "level", logLevel)
os.Exit(3)
return nil, fmt.Errorf("unknown log level: %s", logLevel)

Copilot uses AI. Check for mistakes.
@donatj

donatj commented Jun 24, 2025

Copy link
Copy Markdown
Owner Author

These are breaking changes… I need to consider these

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants