Skip to content

Conversation

@donatj
Copy link
Owner

@donatj donatj commented Jun 24, 2025

No description provided.

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

Copilot AI left a 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 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)

if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
log.Println(ghDelivery, err)
slog.Info("request error", "delivery", ghDelivery, "error", err)
Copy link

Copilot AI Jun 24, 2025

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.
if err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
log.Println(ghDelivery, err)
slog.Info("json decode error", "delivery", ghDelivery, "error", err)
Copy link

Copilot AI Jun 24, 2025

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 on lines +99 to +100
slog.Error("unknown log level", "level", logLevel)
os.Exit(3)
Copy link

Copilot AI Jun 24, 2025

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
Copy link
Owner Author

donatj commented Jun 24, 2025

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.

1 participant