-
Notifications
You must be signed in to change notification settings - Fork 68
Add warning for unused client initialization parameters #3763
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?
Conversation
…parameters Co-authored-by: iscai-msft <[email protected]>
Co-authored-by: iscai-msft <[email protected]>
|
@copilot fix build failures |
Co-authored-by: iscai-msft <[email protected]>
Fixed in commit 3a28190. The build failures were caused by:
Changes made:
All 1098 tests now pass including the 8 new tests for unused client initialization parameter warnings. |
…opilot/fix-unused-client-params-warning
commit: |
|
All changed packages have been documented.
Show changes
|
|
You can try these changes here
|
tadelesh
left a comment
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.
The validation should not apply to all clients since the parameter gathering iteration will cause perf issue. We should check it only when there is customized client initialization parameter. Another problem for this PR is it does not take @paramAlias into account.
| >(context: TCGCContext, client: SdkClientType<TServiceOperation>): void { | ||
| // Only validate when there's a customized @clientInitialization decorator with parameters | ||
| // Skip validation for default/auto-generated client initialization | ||
| if (!client.clientInitialization.__raw) { |
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.
Oh, I just found a problem for this skip. This will also skip the check for nested clients with client initialization customization.
tadelesh
left a comment
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.
@iscai-msft I missed some comments when approval. I could not fine the dismiss button. So, please check my new comments before merge. Thanks.
| for (const path of param.methodParameterSegments) { | ||
| for (const methodParam of path) { | ||
| if (methodParam.kind === "method" && methodParam.onClient) { | ||
| parameterNames.add(methodParam.name); |
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.
You should not add name, but just add instance.
| // Check each custom client initialization parameter | ||
| for (const param of customParams) { | ||
| // Check if this parameter is used in any operation | ||
| if (!allOperationParameterNames.has(param.name)) { |
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.
You could use instance check directly since the methodParameterSegments stores the direct reference, which could resolve potential naming and alias issue.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.