fix(risor,starlark): make WithGlobals additive; drop dead URL check#118
Merged
Conversation
Closes #100 Two unrelated cleanups bundled into the issue. # Part 1: WithGlobals order-dependence `engines/{risor,starlark}/compiler/options.go` had byte-for-byte identical buggy code: WithGlobals(globals []string) — destructive: c.globals = globals WithCtxGlobal() — additive: appends "ctx" if missing So the option order mattered: New(WithGlobals(["x"]), WithCtxGlobal()) → ["x", "ctx"] (works) New(WithCtxGlobal(), WithGlobals(["x"])) → ["x"] (drops ctx) Functional options should be order-independent. Make WithGlobals additive: each call appends, deduplicating names already present. After the fix both orderings yield the same set. The issue body suggested a separate WithGlobalsExact for the "replace-all" semantic; not adding it preemptively (per the user's "backwards compat is not a goal" directive). Callers needing replace-all can construct the slice once and call WithGlobals once. Tests: - "nil globals" / "empty globals" subtests now assert no-op semantics (NotNil + Empty) instead of the prior require.Nil. applyDefaults initializes c.globals to []string{}, and additive nil/empty leaves it untouched. - Three new subtests per engine lock the new contract: - order-independent with WithCtxGlobal (both orderings yield same set) - dedupes within a single call (["a","b","a"] → ["a","b"]) - dedupes across multiple calls (["a","b"] then ["b","c"] → ["a","b","c"]) # Part 2: dead URL check in resolveURL `internal/helpers/requestToMap.go` already carried the comment "tracked for removal under issue #100" on the dead `url.Parse(u.String())` roundtrip. String() is the inverse of Parse for any URL Go produced — the err path is unreachable. Removed the check. With no error path left, the unexported helper's signature simplifies from (*url.URL, error) to *url.URL. Updated the one caller (newHTTPRequestWrapper) and the four resolveURL test subtests to drop the , err return. fmt import stays — RequestToMap itself still wraps errors.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #100 with two small cleanups across the scripting engines and request helpers: it makes WithGlobals order-independent (additive + deduping) for both Risor and Starlark compilers, and removes an effectively unreachable URL round-trip validation in internal/helpers/requestToMap.go by simplifying resolveURL to return a *url.URL directly.
Changes:
- Make
WithGlobals([]string)additive (append + dedupe) in bothengines/risorandengines/starlarkso option order doesn’t dropctxwhen combined withWithCtxGlobal(). - Simplify
resolveURLby removing theurl.Parse(u.String())check and changing its signature from(*url.URL, error)to*url.URL. - Update/extend tests to lock in the new
WithGlobalscontract and reflect the simplifiedresolveURLAPI.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/helpers/requestToMap.go | Removes dead URL re-parse check and simplifies resolveURL + its caller. |
| internal/helpers/requestToMap_test.go | Updates resolveURL tests to match the new non-error-returning signature. |
| engines/starlark/compiler/options.go | Changes WithGlobals to append + dedupe; updates doc comment to reflect new contract. |
| engines/starlark/compiler/options_test.go | Updates nil/empty behavior expectations and adds new tests for ordering and deduping. |
| engines/risor/compiler/options.go | Changes WithGlobals to append + dedupe; updates doc comment to reflect new contract. |
| engines/risor/compiler/options_test.go | Updates nil/empty behavior expectations and adds new tests for ordering and deduping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
robbyt
added a commit
that referenced
this pull request
May 10, 2026
* 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two unrelated cleanups bundled into issue #100.
Part 1 —
WithGlobalsorder-dependence (Risor + Starlark)Both engines had identical buggy code:
WithCtxGlobal()was additive (appendedctxif missing), butWithGlobals()was destructive (overwrote). So option order mattered:Functional options should be order-independent.
WithGlobalsis now additive (append + dedupe), so both orderings produce the same final set.The issue body suggested a separate
WithGlobalsExactfor the "replace-all" semantic — not added preemptively (per the user's "backwards compat is not a goal" directive). Callers needing replace-all can construct the slice once and callWithGlobalsonce.Part 2 — dead URL check in
resolveURLinternal/helpers/requestToMap.goalready carried the comment "tracked for removal under issue #100" on the deadurl.Parse(u.String())roundtrip.String()is the inverse ofParsefor any*url.URLGo produced — the err path is unreachable.Removed the check. With no error path left, the unexported helper's signature simplifies from
(*url.URL, error)to*url.URL. The one caller (newHTTPRequestWrapper) and fourresolveURLtest subtests updated to drop the, errreturn.fmtimport stays —RequestToMapitself still wraps errors.Tests
For
WithGlobals(mirrored across both engines):nil globals/empty globalssubtests now assert no-op semantics (NotNil+Empty) instead of the priorrequire.Nil.applyDefaultsinitializesc.globals = []string{}, and additive nil/empty leaves it untouched.WithCtxGlobal— both orderings yield the same set["a","b","a"]→["a","b"]["a","b"]then["b","c"]→["a","b","c"]Test plan
go test -race -count=1 ./...— greengo vet ./...— cleangit grep -n 'WithGlobalsExact'returns zero hits (we deliberately did not add it)Closes #100
https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Generated by Claude Code