-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 AutoRest C# version #30268
Update AutoRest C# version #30268
Conversation
0b61c93
to
2f1ae38
Compare
2f1ae38
to
f53e922
Compare
sdk/anomalydetector/Azure.AI.AnomalyDetector/src/Generated/AnomalyDetectorModelFactory.cs
Show resolved
Hide resolved
/check-enforcer override |
/// <returns> A new <see cref="Models.ErrorResponse"/> instance for mocking. </returns> | ||
public static ErrorResponse ErrorResponse(string code = null, string message = null) | ||
{ | ||
if (code == 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.
You should use the Argument
helper class in Azure.Core shared source: https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/src/Shared/Argument.cs
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.
Do you mean the generator should do this? This is generated code.
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.
I may be biased since I wrote Argument
, but I used to think the same thing: the generator should use it. However, it was created mostly for dev productivity and as it's grown over the years, think it's fine if the generator does what we see here now. If a project doesn't otherwise link Argument.cs why impact the assembly size unnecessarily? Just my $0.02.
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.
I would tend to agree with you - we don't really need to worry about the productivity of the generator 😄
/// <returns> A new <see cref="Conversations.ConversationTasksState"/> instance for mocking. </returns> | ||
public static ConversationTasksState ConversationTasksState(ConversationTasksStateTasks tasks = null) | ||
{ | ||
if (tasks == 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.
I realize this is already merged, but:
- Why not just let the constructor throw appropriately rather than duplicating logic?
- Even if we duplicate logic, if indeed this is a required parameter why do we still declare it as optional?
* Update AutoRest C# version to 3.0.0-beta.20220804.2 * remove hdinsight * regenerate hdinsight * remove trafficmanager * regenerate trafficmanager Co-authored-by: Arcturus Zhang <dapzhang@microsoft.com>
Update AutoRest C# version to 3.0.0-beta.20220802.1