cleanup: drop redundant nil-handler guards; tighten WithLogHandler doc#114
Conversation
Closes #112 Six call sites guarded `WithLogHandler(cfg.handler)` behind a `cfg.handler != nil` check that existed solely to work around the pre-#111 nil-rejection in `engines/*/compiler/options.go`: - polyscript.go:251-253 (newRisor) - polyscript.go:262-264 (newStarlark) - polyscript.go:273-275 (newExtism) - engines/risor/new.go:72-74 - engines/starlark/new.go:72-74 - engines/extism/new.go:85-87 PR #113 (issue #111) made `WithLogHandler(nil)` a true no-op across all three engines, so each guard now produces identical behavior to an unconditional append. Drop them. Also tightens the top-level `polyscript.WithLogHandler` godoc to match the per-engine wording: "A nil handler is permitted and means 'inherit from slog.Default()' — equivalent to omitting the option." The previous "If unset, the underlying engine picks a default" was vague about the nil case. No new tests required; existing construction tests cover both the nil-handler path (`polyscript_options_test.go:198-205` "WithLogHandler accepts nil") and the explicit-handler path. Full test suite + vet green.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
There was a problem hiding this comment.
Pull request overview
This PR removes redundant cfg.handler != nil guards around WithLogHandler option application in the top-level polyscript.New façade and the per-engine From*Loader constructors, relying on the now-defined “nil handler is a no-op” behavior in the underlying compiler functional options. It also updates the polyscript.WithLogHandler godoc to explicitly describe the nil behavior.
Changes:
- Remove conditional nil-guards and always include
WithLogHandler(cfg.handler)when building engine/compiler option slices. - Reword
polyscript.WithLogHandlerdocumentation to explicitly define nil semantics (“inherit from slog.Default()”).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| polyscript.go | Updates WithLogHandler godoc and simplifies engine option construction by removing redundant nil-guards. |
| engines/risor/new.go | Always passes compiler.WithLogHandler(cfg.handler) in compiler options (removes guard). |
| engines/starlark/new.go | Always passes compiler.WithLogHandler(cfg.handler) in compiler options (removes guard). |
| engines/extism/new.go | Always passes compiler.WithLogHandler(cfg.handler) in compiler options (removes guard). |
Comments suppressed due to low confidence (1)
polyscript.go:193
- The updated doc says a nil handler is equivalent to omitting the option, but the implementation still unconditionally assigns
c.handler = h. This meansWithLogHandler(nonNil)followed byWithLogHandler(nil)will clear the previously configured handler, which is not equivalent to omitting and is inconsistent with the engine-level functional options’ “nil is a no-op and preserves prior state” behavior. Consider makingWithLogHandler(nil)a true no-op (e.g., return a nil Option or skip assignment whenh == nil).
// slog.Default()" — equivalent to omitting the option.
//
// The type parameter is normally inferred from the surrounding [New] call.
func WithLogHandler[E Engine](h slog.Handler) Option[E] {
return func(c *config) { c.handler = h }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* docs: add LOGGING.md describing the logging philosophy Captures the design decisions arrived at across PRs #105, #110, #113, and #114 (issues #85, #99, #111, #112) — the four-PR convergence on: - Logging is opt-in; no constructor demands a slog.Handler. - nil handler means "inherit from slog.Default()" at every layer. - Exactly one fallback path: helpers.SetupLogger. - Functional options are silent on nil (Go idiom). - Group naming follows (engine, struct) shape. Includes a prior-art section showing how each choice tracks stdlib slog, slog.DiscardHandler, OpenTelemetry otelslog, go-logr/logr, the Cheney/Pike functional-options lineage, and database/sql. Also covers the explicitly-rejected alternatives (stdout fallback, nil-warning at construction, panic on nil at option layer, mandatory handler argument, library-defined logger interface) with the failure mode for each. Architectural map shows every nil-handler path funnels through helpers.SetupLogger; history table maps the four issues to the four PRs that shipped them. This is a contributor-facing engineering document. The README's "Capturing diagnostic logs" section is unchanged for now and remains the user-facing entry point; can be linked in a follow-up if desired. * docs(LOGGING): correct three Copilot review findings Three substantive corrections from PR #115 review (Copilot): 1. Principle 1 over-claimed "true no-op on nil" for all four option layers. Only the compiler-level WithLogHandler/WithLogger (relaxed in #111 / PR #113) skip both the set and the clear-of-the-other- field. The top-level and per-engine WithLogHandler do plain field assignment — they accept nil but don't preserve a prior non-nil value across the same option list. Doc now distinguishes the two layers and explains why the distinction is benign for the common single-call case. 2. Principle 2 said records carry the `<engine>.<struct>` prefix "regardless of whether the host configured a handler." That's not true: helpers.SetupLogger only adds the engineName group when the handler is nil. With a host-supplied handler the prefix is just `<struct>`. Doc now describes the asymmetry and explains why it's deliberate (library-owned sink vs. host-owned sink). 3. Principle 5 used `engine=extism` / `script=ExecutableUnit` filter examples that conflate slog groups with key/value attributes. Rephrased to describe slog.Handler.WithGroup behavior accurately (group qualifier on attribute keys; JSON nesting; text prefix) and to condition the `<engine>` prefix on the nil-handler case. The fourth Copilot comment (line 233 — claimed extra leading `|` in the markdown table) was a false positive; the table source has the correct single leading pipe per row and renders as three columns on GitHub. Skipped. --------- Co-authored-by: Claude <noreply@anthropic.com>
* docs: add CHANGELOG.md (Keep a Changelog 1.1.0) Closes #102 Repo had no CHANGELOG.md; users had to read GitHub release notes to find what changed between versions. With v1 ahead and a queue of breaking changes (#86, #87, #88, #89, #90, #91, #104), having a CHANGELOG before landing those gives downstream code reviewers and IDEs visibility into what's coming. Format follows Keep a Changelog 1.1.0 with the standard six headings (Added / Changed / Deprecated / Removed / Fixed / Security). [Unreleased] section captures the run of merged-but-not-released PRs since v0.7.0: - #103 polyscript.New[E] generic constructor (deprecates 12 FromXxx) - #105 slog.Handler optional in engine subpackages - #106 RequestToMap mutation fix - #107 HTTP loader MaxBodySize cap - #110 unify nil-handler on slog.Default(); drop stdout fallbacks - #113 WithLogHandler(nil)/WithLogger(nil) → no-op - #114 drop redundant nil-guards; tighten WithLogHandler doc - #115 docs/LOGGING.md - #117 fix bare top-level json.Number leak - #118 WithGlobals additive; drop dead URL check - #119 extism Eval test coverage Backfilled the three releases the issue called out (v0.5.0, v0.6.0, v0.7.0) from existing GitHub release notes, sorted into Keep a Changelog categories. Earlier releases (v0.0.x through v0.4.0) intentionally not backfilled per the issue scope; can be added in a follow-up if desired. Out of scope: the optional CI gate that fails PRs not touching CHANGELOG.md. Adds noise to small bug PRs and wants opt-out-label infrastructure to support it; better as a separate issue. * docs(CHANGELOG): clarify deprecated FromXxx still take positional handler Copilot review noted that "no constructor demands a handler" is misleading because the deprecated FromXxx constructors still take a positional `logHandler slog.Handler` argument (even though nil is accepted). Reword to distinguish the new generic constructor (no handler arg) from the deprecated ones (still take it, but nil is OK). --------- Co-authored-by: Claude <noreply@anthropic.com>



Summary
Drops the six
cfg.handler != nilguards that wrappedWithLogHandlercalls inpolyscript.goandengines/{risor,starlark,extism}/new.go. They existed solely to work around the pre-#111 nil-rejection — now thatWithLogHandler(nil)is a true no-op across all three engines (PR #113), each guard produces identical behavior to an unconditional append.Also tightens the top-level
polyscript.WithLogHandlergodoc to match the per-engine wording. The previous "If unset, the underlying engine picks a default" was vague about the nil case; the new wording matchesengines/{risor,starlark,extism}/options.go:19-21.Diff shape
Out of scope (audited, no action)
helpers.SetupLoggercallers — 10 sites, all consistent(engine, struct)naming.logHandler slog.Handler+logger *slog.Loggereverywhere.slog.Default().Debugcleanup-error logs inplatform/script/loader/{fromHTTP,fromDisk}.go— these structs have no logger field; routing them throughSetupLoggeris a separate design call.Test plan
git grep -n 'cfg.handler != nil' polyscript.go engines/{risor,starlark,extism}/new.goreturns zero hitsgo test -race -count=1 ./...— greengo vet ./...— cleanpolyscript_options_test.go:198-205"WithLogHandler accepts nil" subtest stays greenCloses #112
https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Generated by Claude Code