-
Notifications
You must be signed in to change notification settings - Fork 790
Fixes #250 - All SDKs Validate Session Config Model #378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 11 commits
596500a
af07711
2a64e9d
e747e18
285e803
8f49e1d
3031fec
f11f1fe
8ad3bbe
a21e19b
c1fed7d
1d40907
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -344,6 +344,19 @@ public async Task<CopilotSession> CreateSessionAsync(SessionConfig? config = nul | |||||
| { | ||||||
| var connection = await EnsureConnectedAsync(cancellationToken); | ||||||
|
|
||||||
| if (!string.IsNullOrEmpty(config?.Model)) | ||||||
| { | ||||||
| // ListModelsAsync caches results after the first call, so this validation has minimal overhead | ||||||
| var availableModels = await ListModelsAsync(cancellationToken).ConfigureAwait(false); | ||||||
|
|
||||||
| if (!availableModels.Any(m => string.Equals(m.Id, config.Model, StringComparison.OrdinalIgnoreCase))) | ||||||
| { | ||||||
| throw new ArgumentException( | ||||||
| $"Invalid model '{config.Model}'. Available models: {string.Join(", ", availableModels.Select(m => m.Id))}", | ||||||
| nameof(config)); | ||||||
|
||||||
| nameof(config)); | |
| nameof(config.Model)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! However, I need to push back on this suggestion. The ArgumentException
constructor's paramName parameter should reference the actual method parameter name, not a
property of that parameter.
According to Microsoft's ArgumentException documentation,
the paramName should be "the name of the parameter that caused the current exception."
In this case:
- The method parameter is
config(of typeSessionConfig?) - There is no parameter named
Modelin the method signature - Using
nameof(config.Model)causes SonarQube warning: "The parameter name 'Model' is not
declared in the argument list"
The error message itself already clearly indicates which property is invalid:
"Invalid model '{config.Model}'. Available models: ..."
This is consistent with standard .NET practices where you reference the parameter, and the
error message provides the specific details about what's wrong with it.
Uh oh!
There was an error while loading. Please reload this page.