drop variadic on AddDataToContext#143
Merged
Merged
Conversation
Signature changes from AddDataToContext(ctx, data ...map[string]any) (context.Context, error) to AddDataToContext(ctx, data map[string]any) (context.Context, error) The variadic was used by exactly one call in the repo (the starlark example, which now merges its two maps before calling). All other ~80 call sites pass a single map; the variadic added nothing for them. Touches the Setter interface, the three Provider implementations (ContextProvider, StaticProvider, CompositeProvider), the two helper wrappers (AddDataToContextHelper, AddDataToContextFromProvider), the three engine Evaluator wrappers, the data-prep starlark example, and ~30 test sites including local mock implementations of data.Setter. Closes #87. 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
Refactors the platform/data runtime-enrichment API by removing the rarely-used variadic parameter from data.Setter.AddDataToContext, simplifying call sites and avoiding unnecessary slice forwarding/looping in provider implementations.
Changes:
- Updated
data.Setter.AddDataToContext(and all implementers/wrappers) to accept a singlemap[string]any. - Updated call sites (including examples and integration tests) to pass one map, merging multiple sources ahead of time.
- Updated mocks/tests and documented the breaking change in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| polyscript_test.go | Updates test mock AddDataToContext signature to non-variadic. |
| polyscript_mocks_test.go | Updates evaluator mock AddDataToContext signature to non-variadic. |
| platform/mocks_test.go | Updates evaluator mock AddDataToContext signature to non-variadic. |
| platform/evaluator_test.go | Updates test mocks and a multi-map call to a single merged map. |
| platform/data/staticProvider.go | Updates StaticProvider.AddDataToContext signature to accept a single map. |
| platform/data/staticProvider_test.go | Updates tests to pass a single map; one subtest name should be renamed to match new API. |
| platform/data/provider.go | Changes Setter interface signature and updates docs/example accordingly. |
| platform/data/provider_test.go | Adjusts tests from multi-map inputs to a single map with multiple keys. |
| platform/data/data_helpers_test.go | Updates MockProvider.AddDataToContext signature to non-variadic. |
| platform/data/contextProvider.go | Simplifies merge logic to iterate over a single map (no outer variadic loop). |
| platform/data/contextProvider_test.go | Updates tests to use a single merged map where applicable. |
| platform/data/compositeProvider.go | Updates composite provider signature and removes variadic forwarding. |
| platform/data/addDataToContext.go | Updates helper/wrapper signatures and removes variadic forwarding. |
| platform/data/addDataToContext_test.go | Updates helper tests to pass a single merged map. |
| examples/data-prep/starlark/main.go | Merges two maps before calling AddDataToContext (adds maps usage). |
| engines/starlark/evaluator/evaluator.go | Updates evaluator wrapper signature and call into data helper. |
| engines/starlark/evaluator/evaluator_test.go | Updates mock provider signature and test inputs to a single map. |
| engines/risor/evaluator/evaluator.go | Updates evaluator wrapper signature and call into data helper. |
| engines/risor/evaluator/evaluator_test.go | Updates mock provider signature and test inputs to a single map. |
| engines/integration_test.go | Updates integration test calls from multi-map to single map literals. |
| engines/extism/evaluator/evaluator.go | Updates evaluator wrapper signature and call into data helper. |
| engines/extism/evaluator/evaluator_test.go | Updates mock providers/test inputs to a single map. |
| CHANGELOG.md | Documents the breaking signature change under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
175
to
179
| @@ -178,9 +178,7 @@ func TestStaticProvider_AddDataToContext(t *testing.T) { | |||
|
|
|||
| newCtx, err := provider.AddDataToContext( | |||
robbyt
changed the title
Per Copilot review on #143 — after dropping the variadic in da45a53 the subtest passes a single multi-key map; the old name was misleading. 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
Drops the variadic on
data.Setter.AddDataToContext. Signature changes fromto
The variadic was used by exactly one call site in the entire repo (the
examples/data-prep/starlarkmain, which now merges its two maps before calling). All other ~80 call sites pass a single map; the variadic added zero value for them and forced an inner loop inContextProvider.AddDataToContextand a forwarding slice throughCompositeProvider.The
(context.Context, error)return stays —ContextProviderstill validates keys and propagatesprocessValueerrors, andStaticProvider'sErrStaticProviderNoRuntimeUpdatessentinel is unchanged.Files touched
platform/data/provider.go— interfaceplatform/data/{contextProvider,staticProvider,compositeProvider,addDataToContext}.go— implementations + helper wrappersengines/{extism,risor,starlark}/evaluator/evaluator.go— wrapper signaturesexamples/data-prep/starlark/main.go— merge two-map call into one (addsmapsimport)_test.gofiles including local mock impls ofdata.SetterCHANGELOG.md—[Unreleased] > ChangedNet diff: +85 / -91 across 23 files.
Closes #87.
Test plan
go build ./...cleango vet ./...cleango test -race -count=1 ./...full suite greengrep -rn 'AddDataToContext(.*\.\.\.\|AddDataToContext(ctx, [^,]*,[^,]*,' --include='*.go'finds no multi-arg callershttps://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Generated by Claude Code