Skip to content
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

Missing cancellation token for Get methods when generating SDK #3330

Closed
mengaims opened this issue Apr 27, 2023 · 7 comments
Closed

Missing cancellation token for Get methods when generating SDK #3330

mengaims opened this issue Apr 27, 2023 · 7 comments
Assignees
Labels
issue-addressed Workflow: The Azure SDK team believes it to be addressed and ready to close. v3 Version 3 of AutoRest C# generator.

Comments

@mengaims
Copy link

Generated code example:
public virtual async Task GetTextBlocklistAsync(string blocklistName, RequestContext context)

The build error example:
image

SDK PR
TypeSpec PR

@mengaims mengaims added the v3 Version 3 of AutoRest C# generator. label Apr 27, 2023
@ArcturusZhang
Copy link
Member

ArcturusZhang commented Apr 30, 2023

I believe this is by-design, this is flagging AZC0002 is because the linter is not properly updated per our decision.
cc @pshao25 @m-nash for more information to share.

As a workaround, please feel free to suppress it @mengaims

@mengaims
Copy link
Author

mengaims commented May 3, 2023

Thanks @ArcturusZhang for the suggestion, I've suppressed AZC0002 and no build error reported now.

@lirenhe lirenhe added the issue-addressed Workflow: The Azure SDK team believes it to be addressed and ready to close. label May 5, 2023
@ghost
Copy link

ghost commented May 5, 2023

Hi @mengaims. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text “/unresolve” to remove the “issue-addressed” label and continue the conversation.

@m-nash
Copy link
Member

m-nash commented May 9, 2023

@ArcturusZhang please open / link an issue in azure-sdk-tools to address updating the analyzer.

@pshao25
Copy link
Member

pshao25 commented May 10, 2023

@m-nash I think we haven't decided how to resolve the ambiguity between convenience and protocol. So we might put off updating the analyzer till we have a decision.

@m-nash
Copy link
Member

m-nash commented May 10, 2023

We have decided.

We will always use overloads which means for protocol we will have all required parameters when an ambiguous call exists i.e. there is no RequestContent or it is optional. When there is a RequestContent we can leave the optional parameters optional.

We are not moving the location of RequestContext param

public Response<Model> Method(string pathParam, CancellationToken token = default)

//protocol must be all required
public Response Method(string pathParam, RequestContext context)

when there is a request content we don't need to make all params required

public Response<Model> Method(string pathParam, InputModel content, CancellationToken token = default)

//protocol doesn't need to be all required
public Response Method(string pathParam, RequestContent content,  RequestContext context = default)

Optional body param

public Response<Model> Method(string pathParam, InputModel content = null, CancellationToken token = default)

//protocol must be all required
public Response Method(string pathParam, RequestContent content,  RequestContext context)

@pshao25
Copy link
Member

pshao25 commented May 11, 2023

Close this as the analyzer update is tracked at Azure/azure-sdk-tools#6145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue-addressed Workflow: The Azure SDK team believes it to be addressed and ready to close. v3 Version 3 of AutoRest C# generator.
Projects
None yet
Development

No branches or pull requests

5 participants