Skip to content

Session.SetModel shouldn't accept a variadic option#904

Open
qmuntal wants to merge 2 commits intogithub:mainfrom
qmuntal:dev/qmuntal/setmodel
Open

Session.SetModel shouldn't accept a variadic option#904
qmuntal wants to merge 2 commits intogithub:mainfrom
qmuntal:dev/qmuntal/setmodel

Conversation

@qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Mar 23, 2026

Session.SetModel accepts an optional SetModelOptions parameter. The correct way of making it optional is to use a pointer, not a variadic argument.

@qmuntal qmuntal requested a review from a team as a code owner March 23, 2026 09:20
Copilot AI review requested due to automatic review settings March 23, 2026 09:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the Go SDK’s Session.SetModel API to remove the variadic options parameter and use a pointer-based options struct, aligning with the repo’s common “nil opts” pattern for optional parameters.

Changes:

  • Change Session.SetModel signature from variadic ...SetModelOptions to *SetModelOptions.
  • Change SetModelOptions.ReasoningEffort from string to *string so it can be omitted explicitly.
  • Update Go E2E tests to pass *SetModelOptions and reasoning-effort pointers.

Reviewed changes

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

File Description
go/session.go Switch SetModel to *SetModelOptions and propagate ReasoningEffort as a pointer; update doc examples.
go/internal/e2e/session_test.go Update SetModel call to pass *SetModelOptions with *string reasoning effort.
go/internal/e2e/rpc_test.go Update SetModel call to pass *SetModelOptions with *string reasoning effort.

@SteveSandersonMS
Copy link
Contributor

@qmuntal Wouldn't this mean that everyone who doesn't want to set options has to explicitly pass nil for the options?

@qmuntal
Copy link
Contributor Author

qmuntal commented Mar 23, 2026

@qmuntal Wouldn't this mean that everyone who doesn't want to set options has to explicitly pass nil for the options?

Yes. Gophers are used to this level of boilerplate due to the lack of optional parameters in Go.

Another option is to not use a pointer, and force callers to instantiate a SetModelOptions struct:

session.SetModel(context.Background(), "gpt-4.1", nil)
// vs
session.SetModel(context.Background(), "gpt-4.1", copilot.SetModelOptions{})

I personally prefer the latter. But the former is more common in the wild, and kind of make it clearer that it is optional.

What for sure it is not idiomatic is to accept a variadic option when the only option is a "fat" struct (one struct that encapsulates all the current -and future- options).

What do you think?

@qmuntal
Copy link
Contributor Author

qmuntal commented Mar 23, 2026

CI test failures seems unrelated to this PR.

@SteveSandersonMS
Copy link
Contributor

What do you think?

Very happy for you to make a call about what's most idiomatic and preferable in Go. I just wanted to be sure I understood.

@qmuntal
Copy link
Contributor Author

qmuntal commented Mar 23, 2026

CI test failures seems unrelated to this PR.

Fixed in #905.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants