refactor(platform): tighten EvaluatorResponse — ExecTime, ScriptExeID, AsMap#141
Open
robbyt wants to merge 4 commits into
Open
refactor(platform): tighten EvaluatorResponse — ExecTime, ScriptExeID, AsMap#141robbyt wants to merge 4 commits into
robbyt wants to merge 4 commits into
Conversation
…, AsMap Three breaking changes to the platform.EvaluatorResponse interface: 1. GetScriptExeID() string → ScriptExeID() string (drop Get prefix) 2. GetExecTime() string → ExecTime() time.Duration (no longer stringified at the boundary; implementations already stored time.Duration internally) 3. New AsMap() (map[string]any, error) — returns the result as a typed map, with an error of the form "AsMap: expected map[string]any, got <T>" when the underlying value has a different shape Migrates 40+ existing `result.Interface().(map[string]any); require.True(t, ok)` test sites to `result.AsMap(); require.NoError(t, err)`, which validates the helper's value. Other typed accessors (AsString, AsBool, AsInt, AsSlice) are not added — they have zero current usage and YAGNI applies. Updates the three implementations (extism, risor, starlark) and all consuming tests/mocks (platform/mocks_test.go, polyscript_mocks_test.go, engines/integration_test.go, readme_test.go, polyscript_*_test.go). Closes #86. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
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 tightens the platform.EvaluatorResponse API by removing Go-anti-idiomatic getter names, returning execution time as time.Duration, and adding a typed AsMap() accessor to reduce Interface().(map[string]any) casting boilerplate across the test suite.
Changes:
- Renamed metadata getters to
ScriptExeID()andExecTime()(nowtime.Duration) across the interface, engine implementations, mocks, and tests. - Added
AsMap() (map[string]any, error)toEvaluatorResponseand implemented it in Extism/Risor/Starlark responses. - Migrated many tests from
Interface().(map[string]any)toAsMap()and updated duration assertions.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
platform/evaluatorResponse.go |
Updates the public EvaluatorResponse interface (rename getters, ExecTime() returns time.Duration, adds AsMap()). |
engines/extism/evaluator/response.go |
Implements ScriptExeID(), ExecTime() time.Duration, and AsMap() for Extism results. |
engines/risor/evaluator/response.go |
Implements ScriptExeID(), ExecTime() time.Duration, and AsMap() for Risor results. |
engines/starlark/evaluator/response.go |
Implements ScriptExeID(), ExecTime() time.Duration, and AsMap() for Starlark results. |
platform/mocks_test.go |
Updates test mocks to satisfy the new interface (including AsMap() and time.Duration exec time). |
polyscript_mocks_test.go |
Updates polyscript test mocks to satisfy the new interface (including AsMap() and time.Duration exec time). |
platform/evaluatorResponse_test.go |
Updates interface conformance tests for renamed methods and duration type. |
platform/evaluator_test.go |
Updates evaluator interface tests to use ScriptExeID() and ExecTime() time.Duration. |
engines/extism/evaluator/response_test.go |
Updates Extism response tests for renamed methods and duration assertions. |
engines/risor/evaluator/response_test.go |
Updates Risor response tests for renamed methods and duration assertions. |
engines/starlark/evaluator/response_test.go |
Updates Starlark response tests for renamed methods and duration assertions. |
engines/extism/evaluator/evaluator_test.go |
Migrates evaluator tests to use AsMap() instead of map type assertions. |
engines/extism/new_test.go |
Migrates end-to-end loader test to use AsMap(). |
engines/integration_test.go |
Migrates integration tests to use AsMap() broadly. |
readme_test.go |
Migrates README example tests to use AsMap(). |
polyscript_test.go |
Migrates polyscript tests to use AsMap(). |
polyscript_options_test.go |
Migrates options tests to use AsMap(). |
polyscript_extism_test.go |
Migrates Extism polyscript tests to use AsMap(). |
CHANGELOG.md |
Documents the breaking changes to EvaluatorResponse and the new AsMap() helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SonarQube reported 30% coverage on new code (required ≥80%) because the AsMap() error branch (non-map underlying value) was unexercised. The 40+ migrated test sites cover only the success path. Adds TestExecResultAsMap to each engine's response_test.go with three subtests: - success: underlying value is a real map - error on string: returns the typed error message - error on nil/int: confirms the %T format string reports the actual type Per-package coverage after these tests: - extism/evaluator: 96.2% - risor/evaluator: 90.8% - starlark/evaluator: 83.5% https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Per Copilot review on #141 — the mock previously discarded the type assertion result and could return (nil, nil) when a test author configured a non-map Return value, silently diverging from the real AsMap contract. The mock now: - Returns (nil, nil) only when explicitly configured with nil + nil - Returns the configured error when the asserted value isn't a map but an error was provided - Synthesizes the production error format ("AsMap: expected map[string]any, got %T") when the asserted value isn't a map and no error was provided Applies to both platform/mocks_test.go and polyscript_mocks_test.go. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Replaces the inline `fmt.Errorf("AsMap: expected map[string]any, got %T", v)`
with a wrapped sentinel in each engine's evaluator package:
- engines/extism/evaluator.ErrAsMapTypeMismatch
= "extism: AsMap expected map[string]any"
- engines/risor/evaluator.ErrAsMapTypeMismatch
= "risor: AsMap expected map[string]any"
- engines/starlark/evaluator.ErrAsMapTypeMismatch
= "starlark: AsMap expected map[string]any"
Each AsMap now returns `fmt.Errorf("%w, got %T", ErrAsMapTypeMismatch, v)`,
so callers can `errors.Is(err, evaluator.ErrAsMapTypeMismatch)` for the
type-mismatch case and still read the concrete Go type from err.Error().
Matches the existing per-package sentinel pattern in
engines/{extism,risor,starlark}/compiler/errors.go.
Tests switched from substring assertions on the full message to
require.ErrorIs(t, err, ErrAsMapTypeMismatch) + a smaller substring
assertion on just the "got <T>" tail.
https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
|
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
Three breaking changes to
platform.EvaluatorResponse:GetScriptExeID() string→ScriptExeID() string— drop theGetprefix per Go styleGetExecTime() string→ExecTime() time.Duration— stop stringifying a Duration at the API boundary; all three implementations already storedtime.DurationinternallyAsMap() (map[string]any, error)— returns the result as a typed map, with an error of the form"AsMap: expected map[string]any, got <T>"when the underlying value has a different shapePer the user's calls: only
AsMapis added (other typed shapes —AsString,AsBool,AsInt,AsSlice— had zero current usage, so YAGNI applies). Return shape iserrorrather than(value, ok).The migration converts 40+ existing
result.Interface().(map[string]any); require.True(t, ok)test sites toresult.AsMap(); require.NoError(t, err), which validates the helper's value.Files touched:
platform/evaluatorResponse.go— interface (addedtimeimport)engines/{extism,risor,starlark}/evaluator/response.go— implementationsplatform/mocks_test.go,polyscript_mocks_test.go— mock impls (addedtimeimport, newAsMap)engines/{extism,risor,starlark}/evaluator/response_test.go— rename calls, switch Duration assertionsengines/integration_test.go,readme_test.go,polyscript_*_test.go,engines/extism/new_test.go,engines/extism/evaluator/evaluator_test.go—AsMap()migrationplatform/evaluatorResponse_test.go,platform/evaluator_test.go— mock setup + Duration assertionsCHANGELOG.md—[Unreleased] > ChangedentryCloses #86.
Test plan
go build ./...cleango vet ./...cleango test -race -count=1 ./...full suite greengrep -rn 'GetExecTime\|GetScriptExeID' --include='*.go' .returns zero hitsgrep -rn 'Interface().(map\[string\]any)' --include='*.go' .returns zero hits (down from 43+)https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Generated by Claude Code