Skip to content

Review logging strategy across the library #52

Description

@harrydayexe

Background

Two different logging strategies currently coexist in the library:

  • pkg/generator — resolves slog.Default() internally inside New(). No logger is injected by the caller.
  • pkg/server — accepts *slog.Logger as a positional argument to New().
  • pkg/watcher — follows the generator approach (slog.Default() resolved internally).

This inconsistency means callers have no uniform mental model: sometimes they supply a logger, sometimes they don't.

Questions to resolve

  1. Should the library always use slog.Default()? This is the simpler approach. Callers configure the default logger once via slog.SetDefault() and every package picks it up automatically. No logger threading required.

  2. Should the library always accept an injected logger? This is more explicit and testable — callers can pass a discard logger in tests without touching global state. It matches the pkg/server pattern.

  3. Should we standardise on one pattern, and if so which? The generator and watcher currently use option 1; the server uses option 2. Mixing them is the current pain point.

Impact

  • If we move pkg/server to slog.Default(), the New(logger, posts, cfg) signature changes — a semver-breaking API change.
  • If we move pkg/generator to accept an injected logger, it adds a positional parameter — also breaking.
  • A non-breaking middle ground: accept an optional logger via config.WithLogger on generator and watcher, keep the positional arg on server, and document the inconsistency as intentional. This is least disruptive but leaves the inconsistency in place.

Suggested approach

Standardise on slog.Default() everywhere and remove the positional logger from pkg/server.New. Log injection for testing is handled by calling slog.SetDefault() in test setup (or wrapping in a t.Cleanup). This simplifies all constructors and matches the generator's existing design.

Should be considered alongside any other planned semver-breaking changes so the signature churn is batched into one release.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions