Skip to content

Commit 8a2cd14

Browse files
robbytclaude
andauthored
refactor(platform): tighten EvaluatorResponse — ExecTime, ScriptExeID, AsMap (#141)
* refactor(platform): tighten EvaluatorResponse — ExecTime, ScriptExeID, 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 * test(evaluator): cover AsMap error paths in all three engines 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 * test(mocks): harden mockEvaluatorResponse.AsMap type-assertion 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 * refactor(evaluator): introduce per-engine ErrAsMapTypeMismatch sentinels 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 * docs(changelog): trim Unreleased entries to match project's terse style Earlier entries (v0.6.0, v0.7.0) keep each item to 1–3 lines of plain facts. The recent additions ballooned into multi-paragraph descriptions with nested bullets and editorial commentary. This brings them back into the established style: 1–3 lines per item, **BREAKING** matching the existing prefix capitalization, just the trailing PR link for traceability. Net change: -41 lines, +20 lines. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c558772 commit 8a2cd14

22 files changed

Lines changed: 364 additions & 166 deletions

CHANGELOG.md

Lines changed: 20 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -42,21 +42,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
4242
appends to the compiler's existing globals, deduplicating names already
4343
present. Order-of-call no longer matters when combined with
4444
`WithCtxGlobal`. ([#118](https://github.com/robbyt/go-polyscript/pull/118))
45-
- **Breaking:** Renamed `machine``engine` in the type-system API to match
46-
the terminology used everywhere else. `ExecutableContent.GetMachineType()`
47-
and `ExecutableUnit.GetMachineType()` are now `EngineType()`. The
48-
`engines/types` package exports `ErrInvalidEngineType`,
49-
`GetEngineTypeFromString`, and `GetEngineTypeFromPath` (renamed from the
50-
`MachineType` equivalents). The conventional import alias is now
51-
`engineTypes`. Closes [#90](https://github.com/robbyt/go-polyscript/issues/90).
45+
- **BREAKING**: Renamed `machine``engine` throughout the type-system API.
46+
`ExecutableContent.GetMachineType()` and `ExecutableUnit.GetMachineType()`
47+
are now `EngineType()`. `engines/types` exports `ErrInvalidEngineType`,
48+
`GetEngineTypeFromString`, and `GetEngineTypeFromPath`.
5249
([#137](https://github.com/robbyt/go-polyscript/pull/137))
53-
- **Breaking:** Unexported the six fields on `platform/script.ExecutableUnit`
54-
(`ID`, `CreatedAt`, `ScriptLoader`, `Compiler`, `Content`, `DataProvider`).
55-
External callers must use the existing getters (`GetID()`, `GetCreatedAt()`,
56-
`GetLoader()`, `GetCompiler()`, `GetContent()`, `GetDataProvider()`) for
57-
reads and construct only via `NewExecutableUnit(...)`. Mid-flight mutation
58-
of `DataProvider` (and the other fields) is no longer possible from outside
59-
the package. Closes [#91](https://github.com/robbyt/go-polyscript/issues/91).
50+
- **BREAKING**: Unexported all six fields on `platform/script.ExecutableUnit`.
51+
Construct via `NewExecutableUnit` only; read via the existing getters.
52+
([#139](https://github.com/robbyt/go-polyscript/pull/139))
53+
- **BREAKING**: Tightened `platform.EvaluatorResponse`. `GetScriptExeID()`
54+
and `GetExecTime()` are now `ScriptExeID()` and `ExecTime() time.Duration`.
55+
Added `AsMap() (map[string]any, error)`; on type mismatch the error wraps
56+
a per-engine `evaluator.ErrAsMapTypeMismatch` sentinel.
57+
([#141](https://github.com/robbyt/go-polyscript/pull/141))
6058

6159
### Deprecated
6260
- The twelve legacy top-level constructors (`FromRisorFile`,
@@ -67,16 +65,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
6765
([#103](https://github.com/robbyt/go-polyscript/pull/103))
6866

6967
### Removed
70-
- **Breaking:** Removed four exported mock types that pulled `testify/mock`
71-
into the production dependency graph: `platform/script/loader.MockLoader`
72-
(with `NewMockLoaderWithContent`), `platform/script.MockCompiler`,
73-
`platform/script.MockExecutableContent`, and the entire `engines/mocks`
74-
package (`Evaluator`, `EvaluatorResponse`). Consumers should copy these
75-
small mock definitions inline into their own `_test.go` files. After this
76-
change `go mod why github.com/stretchr/testify` reports `main module does
77-
not need package github.com/stretchr/testify``testify` is only reached
78-
via test binaries.
79-
Closes [#88](https://github.com/robbyt/go-polyscript/issues/88).
68+
- **BREAKING**: Removed four exported mock types (`loader.MockLoader` with
69+
`NewMockLoaderWithContent`, `script.MockCompiler`,
70+
`script.MockExecutableContent`, and the entire `engines/mocks` package).
71+
`testify` is no longer reachable from any production import path.
8072
([#138](https://github.com/robbyt/go-polyscript/pull/138))
8173
- The redundant `cfg.handler != nil` guards in `polyscript.go` and
8274
`engines/{risor,starlark,extism}/new.go` that existed solely to work
@@ -93,10 +85,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
9385
([#118](https://github.com/robbyt/go-polyscript/pull/118))
9486

9587
### Documentation
96-
- Documented the intentional divergence between Risor and Starlark in how a
97-
callable-returning script is handled: Risor errors, Starlark auto-invokes.
98-
Added a "Script Return Value Handling" section to `engines/README.md` and
99-
brief rationale comments next to each evaluator's branch.
88+
- Documented the intentional Risor vs Starlark divergence for
89+
callable-returning scripts: Risor errors, Starlark auto-invokes. New
90+
"Script Return Value Handling" section in `engines/README.md`.
91+
([#140](https://github.com/robbyt/go-polyscript/pull/140))
10092

10193
### Fixed
10294
- `RequestToMap` no longer mutates the caller's `*http.Request`. The URL

engines/extism/evaluator/errors.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
package evaluator
2+
3+
import "errors"
4+
5+
// ErrAsMapTypeMismatch is returned by (*execResult).AsMap when the underlying
6+
// value is not a map[string]any. Wrapped with the actual Go type via "%T", so
7+
// callers can use errors.Is(err, ErrAsMapTypeMismatch) and still read the
8+
// concrete type from err.Error().
9+
var ErrAsMapTypeMismatch = errors.New("extism: AsMap expected map[string]any")

engines/extism/evaluator/evaluator_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,8 +188,8 @@ func TestEvaluator_Evaluate(t *testing.T) {
188188
require.NotNil(t, response)
189189

190190
// Verify the response
191-
resultMap, ok := response.Interface().(map[string]any)
192-
require.True(t, ok, "Expected map response")
191+
resultMap, err := response.AsMap()
192+
require.NoError(t, err, "Expected map response")
193193
require.Contains(t, resultMap, "result")
194194
require.Equal(t, "success", resultMap["result"])
195195
require.Contains(t, resultMap, "value")

engines/extism/evaluator/response.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func newEvalResult(
3939
func (r *execResult) String() string {
4040
return fmt.Sprintf(
4141
"execResult{Type: %s, Value: %v, ExecTime: %s, ScriptExeID: %s}",
42-
r.Type(), r.value, r.GetExecTime(), r.GetScriptExeID(),
42+
r.Type(), r.value, r.ExecTime(), r.ScriptExeID(),
4343
)
4444
}
4545

@@ -66,12 +66,20 @@ func (r *execResult) Type() data.Types {
6666
}
6767
}
6868

69-
func (r *execResult) GetScriptExeID() string {
69+
func (r *execResult) ScriptExeID() string {
7070
return r.scriptExeID
7171
}
7272

73-
func (r *execResult) GetExecTime() string {
74-
return r.execTime.String()
73+
func (r *execResult) ExecTime() time.Duration {
74+
return r.execTime
75+
}
76+
77+
func (r *execResult) AsMap() (map[string]any, error) {
78+
m, ok := r.value.(map[string]any)
79+
if !ok {
80+
return nil, fmt.Errorf("%w, got %T", ErrAsMapTypeMismatch, r.value)
81+
}
82+
return m, nil
7583
}
7684

7785
func (r *execResult) Inspect() string {

engines/extism/evaluator/response_test.go

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -333,11 +333,11 @@ func TestResponseMethods(t *testing.T) {
333333
handler := slog.NewTextHandler(os.Stdout, nil)
334334
result := newEvalResult(handler, tt.value, tt.execTime, tt.versionID)
335335

336-
// Test GetScriptExeID
337-
assert.Equal(t, tt.versionID, result.GetScriptExeID())
336+
// Test ScriptExeID
337+
assert.Equal(t, tt.versionID, result.ScriptExeID())
338338

339-
// Test GetExecTime
340-
assert.Equal(t, tt.execTime.String(), result.GetExecTime())
339+
// Test ExecTime
340+
assert.Equal(t, tt.execTime, result.ExecTime())
341341
})
342342
}
343343
})
@@ -387,3 +387,34 @@ func TestResponseMethods(t *testing.T) {
387387
}
388388
})
389389
}
390+
391+
func TestExecResultAsMap(t *testing.T) {
392+
t.Parallel()
393+
handler := slog.NewTextHandler(os.Stdout, nil)
394+
395+
t.Run("success", func(t *testing.T) {
396+
m := map[string]any{"k": "v", "n": 42}
397+
result := newEvalResult(handler, m, time.Millisecond, "id-ok")
398+
got, err := result.AsMap()
399+
require.NoError(t, err)
400+
assert.Equal(t, m, got)
401+
})
402+
403+
t.Run("error on string", func(t *testing.T) {
404+
result := newEvalResult(handler, "not a map", time.Millisecond, "id-bad")
405+
got, err := result.AsMap()
406+
require.Error(t, err)
407+
assert.Nil(t, got)
408+
require.ErrorIs(t, err, ErrAsMapTypeMismatch)
409+
assert.Contains(t, err.Error(), "got string")
410+
})
411+
412+
t.Run("error on nil", func(t *testing.T) {
413+
result := newEvalResult(handler, nil, time.Millisecond, "id-nil")
414+
got, err := result.AsMap()
415+
require.Error(t, err)
416+
assert.Nil(t, got)
417+
require.ErrorIs(t, err, ErrAsMapTypeMismatch)
418+
assert.Contains(t, err.Error(), "got <nil>")
419+
})
420+
}

engines/extism/new_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,8 +185,8 @@ func TestFromExtismLoader_RunsEndToEnd(t *testing.T) {
185185

186186
res, err := eval.Eval(t.Context())
187187
require.NoError(t, err)
188-
got, ok := res.Interface().(map[string]any)
189-
require.True(t, ok)
188+
got, err := res.AsMap()
189+
require.NoError(t, err)
190190
assert.Equal(t, "Hello, World!", got["greeting"])
191191
}
192192

0 commit comments

Comments
 (0)