Skip to content

0010. Refactoring hygiene and logging conventions

  • Status: Accepted
  • Date: 2026-06-12
  • Deciders: Ali Irawan

Context

PR #141 (INTR-782) refactored ~20 files to clear SonarCloud maintainability findings. The automated review on that PR surfaced a class of issues that none of our gates (build, tests, SonarCloud quality gate) catch, because each one is locally harmless:

  • extracted helpers inherited fmt.Printf warnings from the original inline code (recordPublishLog), bypassing timestamps and log routing;
  • two packages independently defined a truncate helper with coincidentally equal 500-byte caps (maxLogBodyLen in internal/connectors, previewMaxLen in internal/service) with nothing recording whether the equality is intentional;
  • a helper (loadRevelOutletSecret) returned a value (settings) whose only consumer is the caller's context stash, with a doc comment that didn't say so;
  • a "pure refactor" commit silently removed a string() cast (DisplayType), which is only a no-op because the sqlc-generated field is plain string — a fact the diff alone doesn't prove.

Individually these are nits. Repeated across every future refactor they decay the codebase the same way the original 76 maintainability findings did. We need conventions that direct refactoring work, not one-off fixes.

Decision

We adopt the following conventions for all Go code in this repo, enforced in review:

  1. Operational messages go through the log package. log.Printf (or a structured logger if one is introduced later) for every warning/error destined for operators; fmt.Print* only for CLI/tool output where stdout is the interface (e.g. cmd/). Extracting code into a named function is the moment to fix legacy fmt.Printf calls, not to carry them over.

  2. Coincidental duplication is allowed; silent duplication is not. Small helpers (≤ ~10 lines) may be duplicated across packages when they serve different concerns — we do not create a shared utility package to merge look-alikes whose semantics differ (a log-line cap is not a UI-preview cap). The cost: each copy's driving constant must carry a doc comment stating its purpose and cross-referencing any known look-alike, so future divergence is a conscious choice. Helpers that genuinely share one concern move to pkg/.

  3. Every non-obvious return value is documented. If a function returns something it computed only as a by-product (e.g. parsed settings returned so the caller can stash them in the request context), the doc comment must say who needs it and why. A reviewer should never have to read the call site to learn why a return value exists.

  4. Refactor commits declare any behavior- or type-adjacent change. A commit labeled refactor: must be behavior-preserving. Anything that could change semantics — removing a type conversion, reordering error checks, changing a returned error to a logged one — is either split into its own commit or explicitly called out in the commit message with the evidence (e.g. "row.DisplayType is plain string in database/compiled/models.go; cast removal is a no-op").

  5. Per-goroutine panic recovery uses the named-return + defer recover pattern. As in ImageCopyService.copyItem: the worker helper declares a named result (ok bool), and a deferred recover writes it on panic so the caller can tally outcomes. Each goroutine owns its recovery — an outer recover does not cover spawned goroutines.

Consequences

Positive

  • Refactor PRs have a checklist-shaped contract; review comments shift from re-litigating conventions to verifying them.
  • Log output stays uniform (timestamps, routing) as code moves between inline and extracted forms.
  • Duplicated constants can't drift silently — the cross-reference comment marks every intentional fork.
  • refactor: commits become trustworthy: a reviewer can skim them knowing semantic changes would be declared.

Negative / costs

  • Slightly more ceremony per refactor (doc comments, commit-message declarations, splitting commits).
  • "Allowed duplication" relies on the cross-reference comments being kept accurate; a rename can orphan them.
  • These rules are review-enforced, not tooling-enforced — they depend on reviewers (human or automated) knowing this ADR.

Alternatives considered

  • Shared pkg/truncate (dedupe everything) — rejected: couples unrelated concerns behind one constant; a future change for log lines would silently resize UI previews.
  • Adopt a structured logging library now — out of scope: convention 1 only requires routing through log; a logger migration would be its own ADR.
  • Encode the rules in golangci-lint config committed to the repo — partially possible (forbidigo can ban fmt.Printf outside cmd/), and worth doing later, but rules 2–4 are judgment calls a linter can't make. Left as follow-up rather than blocking this record.

References

  • PR #141 (INTR-782) — the refactor and review that prompted this ADR.
  • Review findings addressed in commit 2e04ecb.
  • Code: internal/service/menu_draft_service.go (recordPublishLog), internal/connectors/dispatch_pipeline.go (maxLogBodyLen), internal/service/adapter_generate_service.go (previewMaxLen), internal/middleware/revel_hmac.go (loadRevelOutletSecret), internal/service/image_copy_service.go (copyItem), service/MenuService/helpers.go (DisplayType).
  • Related: sonar-project.properties exclusion/suppression rationale; ADR 0001 for the adapter-script ES5 conventions.