-
Notifications
You must be signed in to change notification settings - Fork 183
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
Update AZC00002 to match method pair with required requestcontext and optional convenience method #6156
Conversation
optional convenience method
@JoshLove-msft would you be the right person to review this? |
src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs
Outdated
Show resolved
Hide resolved
@m-nash could you review? BTW, since it changes the existing behavior, do you know whether we could run analyzer beforehand against azure-sdk-for-net? |
src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs
Outdated
Show resolved
Hide resolved
@@ -10,7 +10,7 @@ namespace Azure.ClientSdk.Analyzers.Tests | |||
public class AZC0002Tests | |||
{ | |||
[Fact] | |||
public async Task AZC0002ProducedForMethodsWithoutCancellationToken() |
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.
should we have success cases for both cancellation token and request context being last where request context can be both required and optional but cancellation token cannot?
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.
Yes, AZC0002NotProducedForMethodsWithCancellationToken and AZC0002NotProducedForMethodsWithRequestContext
src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs
Show resolved
Hide resolved
public class AZC0017Tests | ||
{ | ||
[Fact] | ||
public async Task AZC0017ProducedForMethodsWithNonOptionalCancellationToken() |
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.
shouldn't this case be caught by AZC0002?
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.
No.
AZC0002: your operations should have last parameter CancellationToken
or RequestContext
.
AZC0017: fine-grained rule for CancellationToken
, your CancellationToken
should optional, and your operation should not have RequestContent
as parameter
AZC0018: fine-grained rule for RequestContext
, your return type should not be a Response<T>
.
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 original intent of AZC0002 was to validate that the last param was CancellationToken AND that it was optional which is why this test existed.
I think we should keep that definition only adding the allowance for RequestContext as well.
AZC0017 should only validate that if the last param is a CancellationToken, then the method MUST NOT contain a parameter of type RequestContent
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.
public Response Method(string name, CancellationToken cancellationToken = default) //PASS
public Response Method(string name, CancellationToken myToken = default) //AZC0002
public Response Method(string name, CancellationToken cancellationToken) //AZC0002
public Response Method(string name, RequestContent content, CancellationToken cancellationToken = default) //AZC0017
public Response Method(string name) //AZC0002
public Response Method(string name) //AZC0002
public Response Method(string name, RequestContext context = default) //AZC0002
public Response Method(string name, RequestContext myContext = default) //AZC0002
public Response Method(string name, RequestContext context) //PASS
//the below two cases require to look in the body for the following line
Argument.AssertNotNull(content, nameof(content));
//If it exists then the context param should be optional else it should be required
public Response Method(string name, RequestContent content, RequestContext context) //variable based on body assert
public Response Method(string name, RequestContent content, RequestContext context = default) //variable based on body assert
public Response Method(string name, RequestContent content, RequestContext context = default) //AZC0018
//future issue
public Response Method(string name, Foo content, RequestContext context = default) //AZC0018 or AZC0019
{ | ||
public class SomeClient | ||
{ | ||
public virtual Task<Response> GetAsync(string s, RequestContext context = null) |
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.
We need to add a test case which has a model as the body param to ensure it throws.
I think you will need to allow any framework types and extensible enum custom types otherwise it should fail?
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.
A protocol method should return Response
, so something like Response<string>
should fail.
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.
For return type yes, I was referring to the body param.
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.
approved with one name typo
[Fact] | ||
public async Task AZC0002ProducedWhenCancellationTokenOverloadsDontMatch() | ||
public async Task AZC0002NotProducedForMethodsWithRequestContentAndCancellationToken() |
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.
Is this method named incorrectly shouldn't it be `AZC0002NotProducedForMethodsWithRequestContextAndCancellationToken
Fix #6145