make slog.Handler optional in engine subpackages; drop dead engines.NewEvaluator dispatcher by robbyt · Pull Request #105 · robbyt/go-polyscript

robbyt · 2026-05-06T05:59:53Z

Summary

Closes #85.

polyscript.New[E] (shipped in PR #103) already treats slog.Handler as optional via WithLogHandler[E]. This PR finishes the job for the per-engine subpackages, which still required handler as a positional argument and duplicated From*Loader / From*LoaderWithData / NewEvaluator constructors.

Per-engine subpackage shape

Each of engines/risor, engines/starlark, engines/extism now exposes a single options-driven entry point:

risor.FromRisorLoader(ldr,
    risor.WithLogHandler(handler),
    risor.WithStaticData(map[string]any{"name": "World"}),
)

extism.FromExtismLoader(ldr,
    extism.WithEntryPoint("greet"),
    extism.WithStaticData(map[string]any{"input": "World"}),
)

Each subpackage has the same options:

WithLogHandler(slog.Handler) — nil-tolerant; defaults to slog.Default()
WithStaticData(map[string]any) — composes a StaticProvider + ContextProvider
WithDataProvider(data.Provider) — bypasses composition; takes precedence over WithStaticData
WithEntryPoint(string) — Extism only; required (returns ErrEntryPointRequired if missing)

The 6 redundant constructors (From*LoaderWithData × 3, NewEvaluator × 3) are removed.

Single source of truth for default handler

internal/helpers.SetupLogger now inherits from slog.Default() when handler is nil. The redundant stderr fallback in each compiler's applyDefaults is removed, so there's one fallback site instead of four.

`engines.NewEvaluator` / `engines.NewCompiler` removed

These public dispatchers in engines/new.go had zero callers outside their own test file — polyscript.New[E] supersedes them. Deleted along with their generator templates and typeGen.go entries.

`deprecated.go` unchanged externally

The 12 deprecated top-level constructors keep their existing positional (handler, ...) signatures; their bodies translate internally into the new options form. A separate v1 issue (#104) tracks deletion.

Files

31 files changed, +1103 / −1585
New: engines/{risor,starlark,extism}/options.go + doc.go, internal/helpers/logger_test.go
Deleted: engines/new.go, engines/new_test.go, two .tmpl files
README has a new "Direct engine subpackage usage" section

Test plan

go test -race -count=1 ./... — all green
go vet ./... — clean
Per-engine new_test.go rewritten with no-handler / nil-handler / DataProvider / EntryPoint coverage
internal/helpers/logger_test.go covers nil-handler default-inheritance, custom handler honored, group prefix behavior
platform/script/executableUnit_test.go adds a nil-handler regression test
deprecated_test.go unchanged — locks in that the public deprecated surface still works
CI: GitHub Actions + SonarCloud (waiting on PR)
Copilot review (waiting on PR)

https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL

Generated by Claude Code

…ewEvaluator dispatcher (#85) Each engine subpackage now exposes a single FromXLoader(ldr, opts...) constructor with engine-scoped options: - WithLogHandler / WithStaticData / WithDataProvider on all three - WithEntryPoint on engines/extism (required) The four FromXLoaderWithData/NewEvaluator variants per engine fold into the new options form. polyscript.New[E] and deprecated.go translate to the new shape internally; the deprecated public surface is unchanged. Default-handler logic moves to a single source of truth: internal/helpers.SetupLogger now inherits from slog.Default() when given nil. The redundant stderr fallback in each compiler's applyDefaults is removed. engines/new.go's NewEvaluator/NewCompiler dispatchers were dead public API (no callers outside the file's own test); both they and the generator templates that produce them are deleted. Closes #85

github-actions · 2026-05-06T06:00:04Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

…ns, gofmt integration_test.go Addresses six golangci-lint findings on PR #105: - usetesting: replace context.Background() with t.Context() in the new RunsEndToEnd tests under engines/{risor,starlark,extism}/new_test.go - testifylint require-error: switch assert.ErrorIs to require.ErrorIs in TestFromExtismLoader_RequiresEntryPoint and TestFromExtismLoader_EmptyEntryPointStillRejected - gofmt: re-align over-indented option args in integration_test.go Drops the now-unused context import in the three new_test.go files.

Sonar S3776: New[E]'s 20-rated complexity exceeded the 15 budget. Moves the per-engine option-building blocks into newRisor/newStarlark/newExtism helpers, leaving New[E] as a thin dispatcher. No behavior change.

Copilot

Pull request overview

This PR completes the migration to an options-driven construction API for the per-engine subpackages (risor/starlark/extism), making slog.Handler optional (nil-tolerant) and removing redundant constructors and the unused engines.NewEvaluator/engines.NewCompiler dispatch surface. It also centralizes default logging fallback behavior in internal/helpers.SetupLogger and updates docs/tests accordingly.

Changes:

Replace per-engine positional constructors with FromXxxLoader(ldr, opts...) plus WithLogHandler / WithStaticData / WithDataProvider (and Extism WithEntryPoint) options.
Centralize nil-handler fallback to inherit from slog.Default() in internal/helpers.SetupLogger; remove compiler stderr defaults.
Remove generated engines/new.go dispatcher and its generator templates; update README and tests to match the new APIs.

Reviewed changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
README.md	Adds “Direct engine subpackage usage” docs showing the new per-engine options API.
polyscript.go	Updates `polyscript.New[E]` to call per-engine `FromXxxLoader` with options and omit handler when nil.
platform/script/executableUnit.go	Documents and relies on nil-handler support via `helpers.SetupLogger`.
platform/script/executableUnit_test.go	Adds regression coverage ensuring `NewExecutableUnit` works with a nil handler.
internal/helpers/logger.go	Changes default logging fallback to inherit from `slog.Default().Handler()` when handler is nil.
internal/helpers/logger_test.go	Adds tests covering default inheritance and grouping behavior in `SetupLogger`.
engines/types/gen/typeGen.go	Stops generating `engines/new.go` and `engines/new_test.go`.
engines/types/gen/templates/new.go.tmpl	Deleted: generator template for the `engines` dispatcher.
engines/types/gen/templates/new_test.go.tmpl	Deleted: generator template for the `engines` dispatcher tests.
engines/starlark/options.go	New: defines the engine-scoped options for `FromStarlarkLoader`.
engines/starlark/new.go	Replaces multiple constructors with a single options-driven `FromStarlarkLoader`.
engines/starlark/new_test.go	Rewrites tests to cover nil/no handler, options precedence, and end-to-end evaluation.
engines/starlark/doc.go	New: package docs for direct engine-level usage and options.
engines/starlark/compiler/options.go	Removes stderr default handler; defers nil-handler behavior to `helpers.SetupLogger`.
engines/starlark/compiler/options_test.go	Updates expectations around logging defaults/validation after deferring to `SetupLogger`.
engines/risor/options.go	New: defines the engine-scoped options for `FromRisorLoader`.
engines/risor/new.go	Replaces multiple constructors with a single options-driven `FromRisorLoader`.
engines/risor/new_test.go	Rewrites tests to cover nil/no handler, options precedence, and end-to-end evaluation.
engines/risor/doc.go	New: package docs for direct engine-level usage and options.
engines/risor/compiler/options.go	Removes stderr default handler; defers nil-handler behavior to `helpers.SetupLogger`.
engines/risor/compiler/options_test.go	Updates expectations around logging defaults/validation after deferring to `SetupLogger`.
engines/new.go	Deleted: public dispatcher API that had no external callers.
engines/new_test.go	Deleted: tests for the removed `engines` dispatcher API.
engines/integration_test.go	Updates integration tests to use the new per-engine options constructors.
engines/extism/options.go	New: defines the engine-scoped options for `FromExtismLoader`, including `WithEntryPoint`.
engines/extism/new.go	Replaces multiple constructors with a single options-driven `FromExtismLoader` and adds `ErrEntryPointRequired`.
engines/extism/new_test.go	Rewrites tests to cover entry point validation, nil/no handler, options precedence, and end-to-end evaluation.
engines/extism/doc.go	New: package docs for direct engine-level usage and options (including required entry point).
engines/extism/compiler/options.go	Removes stderr default handler; defers nil-handler behavior to `helpers.SetupLogger`.
engines/extism/compiler/options_test.go	Updates expectations around logging defaults/validation after deferring to `SetupLogger`.
deprecated.go	Keeps deprecated top-level constructors but translates them into the new per-engine options form.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Address Copilot review feedback on PR #105: - engines/{risor,starlark,extism}/new.go: WithLogHandler on the loader was reaching the evaluator and ExecutableUnit but not the compiler, so compiler-level logs still went through slog.Default(). Now the compiler is constructed with compiler.WithLogHandler(cfg.handler) when non-nil. Renamed the local 'compiler' variable to 'comp' to unshadow the imported compiler package. - internal/helpers/logger.go: SetupLogger doc previously implied engineName was always added as a slog group. Clarify that engineName is only applied when handler is nil (the implicit-default branch); when an explicit handler is passed, the caller is presumed to have configured grouping the way they want. Switched the doc reference from [slog.Default] to slog.Default().Handler(). - engines/{risor,starlark,extism}/new_test.go and internal/helpers/logger_test.go: add a comment on each SetDefault- using test marking it non-parallel-safe (per-package mutation must be serialized; cross-package isolation comes from one process per package under `go test ./...`).

Copilot

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.

…e, clarify logger doc - polyscript.New[Extism] now wraps extism.ErrEntryPointRequired so callers can detect the condition with errors.Is across both APIs - README Extism example is self-contained: constructs a WASM-appropriate loader (loader.NewFromBytes) with required imports - SetupLogger doc references the actual <engineName>.<...> prefix instead of a literal "engine.<...>"

sonarqubecloud · 2026-05-07T04:45:15Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

PR #105 (issue #85) made SetupLogger inherit from slog.Default() when given nil, but three constructors still wrote a fresh TextHandler to os.Stdout in that case: - engines/extism/evaluator/response.go newEvalResult - engines/risor/evaluator/response.go newEvalResult - engines/starlark/evaluator/response.go newEvalResult That's the exact "library writes to stdout when nil-handler" pattern issue #99 was filed against. Replace each with a single helpers.SetupLogger(handler, "<engine>", "execResult") call so nil now inherits from slog.Default() like the rest of the package, and the "Handler is nil, using the default logger configuration" warning goes away. The (engine, execResult) group structure is preserved. Also drops three stale TODOs: - platform/data/addDataToContext.go: "// TODO: remove or use logger more effectively" — the slog.Default() fallback is the right behavior post-#85; the TODO is a leftover. - engines/extism/compiler/compiler.go: "TODO: error paths hard to test" — converted to issue #108 with concrete branch list and a proposed adapter-mock approach. - engines/extism/evaluator/evaluator.go: same TODO — converted to issue #109 with the equivalent treatment for Eval/exec/execHelper. Coverage on extism compiler/evaluator is ~93%; the remaining branches need adapter-level mocking, tracked under #108/#109. Closes #99 Co-authored-by: Claude <noreply@anthropic.com>

* 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>

Copilot AI review requested due to automatic review settings May 6, 2026 05:59

Copilot started reviewing on behalf of robbyt May 6, 2026 06:00 View session

claude added 2 commits May 6, 2026 06:02

extract per-engine helpers from New[E] to drop cognitive complexity

aa9b318

Sonar S3776: New[E]'s 20-rated complexity exceeded the 15 budget. Moves the per-engine option-building blocks into newRisor/newStarlark/newExtism helpers, leaving New[E] as a thin dispatcher. No behavior change.

Copilot AI reviewed May 6, 2026

View reviewed changes

claude added 3 commits May 6, 2026 06:08

remove stale 'what changed' comments from issue-85 changes

399e5be

fold engine package godoc into new.go; delete doc.go

e589caa

robbyt requested a review from Copilot May 7, 2026 04:36

Copilot started reviewing on behalf of robbyt May 7, 2026 04:37 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

Comment thread README.md Outdated

Comment thread polyscript.go Outdated

Comment thread internal/helpers/logger.go Outdated

robbyt merged commit 0d35bd8 into main May 7, 2026
3 checks passed

robbyt deleted the fix/issue-85-optional-handler branch May 7, 2026 06:29

robbyt mentioned this pull request May 7, 2026

fix: unify nil-handler handling on slog.Default(); drop stale TODOs #110

Merged

4 tasks

robbyt mentioned this pull request May 8, 2026

docs: add LOGGING.md describing the logging philosophy #115

Merged

3 tasks

robbyt mentioned this pull request May 10, 2026

docs: add CHANGELOG.md #120

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make slog.Handler optional in engine subpackages; drop dead engines.NewEvaluator dispatcher#105

make slog.Handler optional in engine subpackages; drop dead engines.NewEvaluator dispatcher#105
robbyt merged 7 commits into
mainfrom
fix/issue-85-optional-handler

robbyt commented May 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 6, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud Bot commented May 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

robbyt commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Per-engine subpackage shape

Single source of truth for default handler

engines.NewEvaluator / engines.NewCompiler removed

deprecated.go unchanged externally

Files

Test plan

Uh oh!

github-actions Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

Scanned Files

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud Bot commented May 7, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robbyt commented May 6, 2026 •

edited

Loading

`engines.NewEvaluator` / `engines.NewCompiler` removed

`deprecated.go` unchanged externally

github-actions Bot commented May 6, 2026 •

edited

Loading