-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 #21225
Update AutoRest C# version #21225
Conversation
51241a1
to
1ff43e0
Compare
This requires manual changes for the security attribute change. |
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them. In order to bootstrap pipelines for a new service, please perform following steps: For data-plane/track 2 SDKs Issue the following command as a pull request comment:
For track 1 management-plane SDKsPlease open a separate PR and to your service SDK path in this file. Once that PR has been merged, you can re-run the pipeline to trigger the verification. |
11989ee
to
f5778e6
Compare
Blew away my work, again :( |
58d3b80
to
6438194
Compare
Failing with:
|
6438194
to
1e1db40
Compare
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.
Event Grid changes look good.
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 factory isn't really needed in either KV project. See comments for details.
@@ -0,0 +1,169 @@ | |||
// Copyright (c) Microsoft Corporation. All rights reserved. |
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.
This is a sample and doesn't need this. I'd rather not have to maintain it or complicate the sample with unnecessary 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.
If this can't be elided, can you at least use the CodeModelAttribute
to make it internal. I'd rather not have it in the sample, but not exposing it is next best.
@@ -0,0 +1,53 @@ | |||
// Copyright (c) Microsoft Corporation. All rights reserved. |
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.
This doesn't add any value since all classes are constructable with public members. Can we suppress this for now? I wouldn't want to call the class name this anyway, as it doesn't follow our existing naming convention we used in other packages.
For example, can we use existing model renaming functionality to not only rename this but also hide it?
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'll make it internal for now. Do you need an ability to be able to remove it completely?
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.
Don't need it, but definitely need the ability to rename it. That is not the naming convention we have used throughout all the languages SDKs for KV.
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.
Given what you told Mariana, could you rename this to just AdministrationModelFactory
. That aligns with our other model factories in our other KV packages.
I believe there is a key @AlexanderSher can add to your project to disable it. |
namespace Azure.Storage.Queues | ||
{ | ||
/// <summary> Model factory for AzureQueueStorage read-only models. </summary> | ||
public static partial class AzureQueueStorageModelFactory |
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.
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.
Could the generator define a new attribute like CodeModelAttribute
where, if provided, you use that name? That would also help resolve one of my comments.
1e1db40
to
d3a59b9
Compare
I have removed flag in the last commit, but I can bring it back. However, adding an ability to remove specific types from codegen sounds like a better, though more complicated approach. |
using Azure.AI.FormRecognizer.Models; | ||
using Azure.AI.FormRecognizer.Training; | ||
|
||
namespace Azure.AI.FormRecognizer |
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 there a way to change the namespace? what is the repercussion for this in FR? https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/formrecognizer/Azure.AI.FormRecognizer/src/FormRecognizerModelFactory.cs#L11
Also, we are in code freeze for anything that modifies the public API for FR until we go GA next week
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, you can change namespace and type name similar to REST client:
[CodeGenType("GeneratedModelFactory")]
partial class MyModelFactory
/// <param name="documentId"> Document Id. </param> | ||
/// <param name="charactersCharged"> Character charged by the API. </param> | ||
/// <returns> A new <see cref="Document.DocumentStatusResult"/> instance for mocking. </returns> | ||
public static DocumentStatusResult DocumentStatusResult(Uri translatedDocumentUri = default, Uri sourceDocumentUri = default, DateTimeOffset createdOn = default, DateTimeOffset lastModified = default, TranslationStatus status = default, string translateTo = default, DocumentTranslationError error = default, float progress = default, string documentId = default, long charactersCharged = default) |
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.
curious, why there are only 2 types generated here?
also, why is everything with =default?
/// <param name="certainty"> Describes the entities certainty and polarity. </param> | ||
/// <param name="association"> Describes if the entity is the subject of the text or if it describes someone else. </param> | ||
/// <returns> A new <see cref="TextAnalytics.HealthcareEntityAssertion"/> instance for mocking. </returns> | ||
public static HealthcareEntityAssertion HealthcareEntityAssertion(EntityConditionality? conditionality = default, EntityCertainty? certainty = default, EntityAssociation? association = default) |
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 feel like there are a ton of other types missing. do you know why is that? i.e. HealthcareEntity
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.
HealthcareEntity
is hand-written type, the one that is generated is HealthcareEntityInternal
:
azure-sdk-for-net/sdk/textanalytics/Azure.AI.TextAnalytics/src/Internal/HealthcareEntityInternal.cs
Line 12 in 006cfd3
internal partial class HealthcareEntityInternal { } |
This type is made internal, so there can be no public factory method for it.
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.
tables looks good.
d3a59b9
to
8f88549
Compare
|
||
namespace Azure.Security.KeyVault.Administration | ||
{ | ||
/// <summary> Model factory for AzureSecurityKeyVaultAdministration read-only models. </summary> |
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 name in the comment doesn't make sense. I'd just elide it from generated code and leave the comment "Model factory for read-only models." The package name really doesn't add much value to the statement anyway.
Fixes #21144 |
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.
Attestation looks good.
aa80d4a
to
1724e32
Compare
namespace Azure.Search.Documents | ||
{ | ||
/// <summary> Model factory for SearchService read-only models. </summary> | ||
public static partial class SearchServiceModelFactory |
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.
Noob questions here -
- What is this needed for?
- Will I manually update this file if a change is needed (like yanking out some model type)?
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.
These classes are only for customer tests e.g. for mocking - to create models that have members customers can't otherwise set because they are response-only, for example. See https://azure.github.io/azure-sdk/dotnet_introduction.html#dotnet-mocking for more details.
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.
Thanks Heath.
1724e32
to
5187587
Compare
Replaced with #21301 to unblock other work in Autorest |
Update AutoRest C# version to 3.0.0-beta.20210520.2