TestV2 for patch model purpose#7
TestV2 for patch model purpose#7pshao25 wants to merge 5 commits intoannelo-msft:core-mergepatch-proto2from
Conversation
| public class TestV2ClientTest | ||
| { | ||
| [Test] | ||
| public async Task PathTestAsync() |
There was a problem hiding this comment.
Should this be PatchTest? I just want to make sure I understand the intention of this test and what it is trying to illustrate.
| // TODO: was there a good way to get RequestContext without creating it new? | ||
| RequestContext context = new() { CancellationToken = cancellationToken }; | ||
|
|
||
| Response response = await PatchAsync(testV2.Key, content, context).ConfigureAwait(false); |
There was a problem hiding this comment.
How do you know that testV2.Key will have a value if it is not required by the model constructor?
| { | ||
| // RequiredProperty could not set | ||
| TestV2 testV2 = new TestV2(); | ||
| testV2.Key = "key"; |
There was a problem hiding this comment.
If we add "key" to the model after it is constructed, it will be added to the request body payload. I think this is incorrect - a PATCH operation should not allow you to change the unique id of a resource.
There was a problem hiding this comment.
I opened up an issue to discuss Azure/autorest.csharp#3640
| /// public TestV2 CreateEmptyTestV2() | ||
| /// just for patch usage. | ||
| /// </summary> | ||
| public TestV2() |
There was a problem hiding this comment.
This should require any required properties.
| /// @visibility("read") | ||
| /// readOnlyProperty: string; // Add this propery to differentiate request from response. | ||
| /// optionalProperty?: string; | ||
| /// requiredProperty: int32; |
There was a problem hiding this comment.
We should understand why this property is required for GET and PUT but not for PATCH. I don't think we have a good foundation for this question yet.
2 participants
All SDK Contribution checklist:
This checklist is used to make sure that common guidelines for a pull request are followed.
Draftmode if it is:General Guidelines and Best Practices
Testing Guidelines
SDK Generation Guidelines
*.csprojandAssemblyInfo.csfiles have been updated with the new version of the SDK. Please double check nuget.org current release version.Additional management plane SDK specific contribution checklist:
Note: Only applies to
Microsoft.Azure.Management.[RP]orAzure.ResourceManager.[RP]Management plane SDK Troubleshooting
If this is very first SDK for a services and you are adding new service folders directly under /SDK, please add
new servicelabel and/or contact assigned reviewer.If the check fails at the
Verify Code Generationstep, please ensure:generate.ps1/cmdto generate this PR instead of callingautorestdirectly.Please pay attention to the @microsoft.csharp version output after running
generate.ps1. If it is lower than current released version (2.3.82), please run it again as it should pull down the latest version.Note: We have recently updated the PSH module called by
generate.ps1to emit additional data. This would help reduce/eliminate the Code Verification check error. Please run following command:Old outstanding PR cleanup
Please note:
If PRs (including draft) has been out for more than 60 days and there are no responses from our query or followups, they will be closed to maintain a concise list for our reviewers.