Add autorest-generated model factories #21301

AlexanderSher · 2021-05-24T20:34:15Z

JoshLove-msft

Event Grid updates look good.

maririos · 2021-05-24T22:52:04Z

sdk/formrecognizer/Azure.AI.FormRecognizer/api/Azure.AI.FormRecognizer.netstandard2.0.cs

@@ -337,7 +337,9 @@ public static partial class FormRecognizerModelFactory
        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
        public static Azure.AI.FormRecognizer.Training.CustomFormModel CustomFormModel(string modelId, Azure.AI.FormRecognizer.Training.CustomFormModelStatus status, System.DateTimeOffset trainingStartedOn, System.DateTimeOffset trainingCompletedOn, System.Collections.Generic.IReadOnlyList<Azure.AI.FormRecognizer.Training.CustomFormSubmodel> submodels, System.Collections.Generic.IReadOnlyList<Azure.AI.FormRecognizer.Training.TrainingDocumentInfo> trainingDocuments, System.Collections.Generic.IReadOnlyList<Azure.AI.FormRecognizer.Models.FormRecognizerError> errors) { throw null; }
        public static Azure.AI.FormRecognizer.Training.CustomFormModel CustomFormModel(string modelId, Azure.AI.FormRecognizer.Training.CustomFormModelStatus status, System.DateTimeOffset trainingStartedOn, System.DateTimeOffset trainingCompletedOn, System.Collections.Generic.IReadOnlyList<Azure.AI.FormRecognizer.Training.CustomFormSubmodel> submodels, System.Collections.Generic.IReadOnlyList<Azure.AI.FormRecognizer.Training.TrainingDocumentInfo> trainingDocuments, System.Collections.Generic.IReadOnlyList<Azure.AI.FormRecognizer.Models.FormRecognizerError> errors, string modelName, Azure.AI.FormRecognizer.Training.CustomFormModelProperties properties) { throw null; }
+        public static Azure.AI.FormRecognizer.Training.CustomFormModelField CustomFormModelField(string name = null, float? accuracy = default(float?)) { throw null; }


FR is in code freeze until we release GA (hopefully this week). Is there a way to disable the changes here?

I can add an internal type that will make codegen MF internal. We will delete it after GA.

heaths

Class names need to be changed to match pattern, and the sample factory isn't necessary because it's a sample. These classes just complicate it unnecessarily. Can we just turn off factory generation?

...t/Azure.Security.KeyVault.Administration/src/Generated/KeyVaultAdministrationModelFactory.cs

sdk/keyvault/samples/sharelink/KeyVaultStorageModelFactory.cs

heaths · 2021-05-24T23:25:44Z

sdk/keyvault/Azure.Security.KeyVault.Administration/src/KeyVaultAdministrationModelFactory.cs

 using Azure.Security.KeyVault.Administration.Models;

 namespace Azure.Security.KeyVault.Administration
 {
    /// <summary>
    /// A factory class which constructs model classes for mocking purposes.
    /// </summary>
-    public static class KeyVaultAdministrationModelFactory
+    [CodeGenType("AzureSecurityKeyVaultAdministrationModelFactory")]
+    public static partial class KeyVaultAdministrationModelFactory


Just AdministrationModelFactory, which matches our other factory classes we already GA'd.

This type was there already. Do you want me to rename it?

Oh, wow, you're right. I didn't realize that (or just flat-out forgot). @christothes it seems to me that we should leave this name as-is since "AdministrationModelFactory" is rather vague, even if it fits the existing pattern. Agreed?

Giving timing, we'll leave it as-is for now. If there's concern, we still have a little time to change it.

AlexanderSher · 2021-05-25T17:35:16Z

Is there anybody else who needs public API changes to be reverted in this PR?

…Token doesn't throw NRE anymore.

annelo-msft · 2021-05-25T21:50:59Z

.../Azure.Containers.ContainerRegistry/api/Azure.Containers.ContainerRegistry.netstandard2.0.cs

+    public static partial class ContainerRegistryModelFactory
+    {
+        public static Azure.Containers.ContainerRegistry.ArtifactManifestProperties ArtifactManifestProperties(string registryLoginServer = null, string repositoryName = null, string digest = null, long? size = default(long?), System.DateTimeOffset createdOn = default(System.DateTimeOffset), System.DateTimeOffset lastUpdatedOn = default(System.DateTimeOffset), Azure.Containers.ContainerRegistry.ArtifactArchitecture? architecture = default(Azure.Containers.ContainerRegistry.ArtifactArchitecture?), Azure.Containers.ContainerRegistry.ArtifactOperatingSystem? operatingSystem = default(Azure.Containers.ContainerRegistry.ArtifactOperatingSystem?), System.Collections.Generic.IReadOnlyList<Azure.Containers.ContainerRegistry.ArtifactManifestReference> manifestReferences = null, System.Collections.Generic.IReadOnlyList<string> tags = null, bool? canDelete = default(bool?), bool? canWrite = default(bool?), bool? canList = default(bool?), bool? canRead = default(bool?), string quarantineState = null, string quarantineDetails = null) { throw null; }
+        public static Azure.Containers.ContainerRegistry.ArtifactManifestReference ArtifactManifestReference(string digest = null, Azure.Containers.ContainerRegistry.ArtifactArchitecture architecture = default(Azure.Containers.ContainerRegistry.ArtifactArchitecture), Azure.Containers.ContainerRegistry.ArtifactOperatingSystem operatingSystem = default(Azure.Containers.ContainerRegistry.ArtifactOperatingSystem)) { throw null; }


If we ever wanted to use EditorBrowsable Never here, would that get picked up on the creation methods automatically?

If you apply [EditorBrowsable(EditorBrowsableState.Never)] and [Browsable(false)] to the hand-written partial part of the factory, it should affect all the methods in it.

annelo-msft

ACR looks good - thanks!

heaths · 2021-05-25T22:23:05Z

@AlexanderSher

Is there anybody else who needs public API changes to be reverted in this PR?

Actually, KV is about to GA in a couple weeks, so if that's an option, yes, I'd rather not take these changes. That also gives us time to work out the issues. I still think dropping the redundant class name from the class summary should go in before this one (or soon after, though). It's currently out of sync with the class name, and really is redundant since it's literally in the class summary.

- Remove unwanted model factories

AlexanderSher · 2021-05-26T23:19:01Z

We have added assembly-level attribute CodeGenSuppressTypeAttribute that allows you to suppress generation of any type (not just model factory).
@maririos , you'll need to remove this usage after GA:
https://github.com/Azure/azure-sdk-for-net/pull/21301/files#diff-e7e5f4324b0c386a49f3bcbc03c54b0159cca15ad2918c24521b3eeb12f77b2bR13

@heaths, codegen'ed model factories are removed.

heaths · 2021-05-26T23:42:19Z

sdk/keyvault/Azure.Security.KeyVault.Administration/src/KeyVaultAdministrationModelFactory.cs

 using Azure.Security.KeyVault.Administration.Models;

 namespace Azure.Security.KeyVault.Administration
 {
    /// <summary>
    /// A factory class which constructs model classes for mocking purposes.
    /// </summary>
-    public static class KeyVaultAdministrationModelFactory
+    [CodeGenType("AzureSecurityKeyVaultAdministrationModelFactory")]
+    public static partial class KeyVaultAdministrationModelFactory


Oh, wow, you're right. I didn't realize that (or just flat-out forgot). @christothes it seems to me that we should leave this name as-is since "AdministrationModelFactory" is rather vague, even if it fits the existing pattern. Agreed?

sdk/keyvault/Azure.Security.KeyVault.Administration/src/Properties/AssemblyInfo.cs

heaths · 2021-05-27T00:24:49Z

sdk/keyvault/Azure.Security.KeyVault.Administration/src/KeyVaultAdministrationModelFactory.cs

 using Azure.Security.KeyVault.Administration.Models;

 namespace Azure.Security.KeyVault.Administration
 {
    /// <summary>
    /// A factory class which constructs model classes for mocking purposes.
    /// </summary>
-    public static class KeyVaultAdministrationModelFactory
+    [CodeGenType("AzureSecurityKeyVaultAdministrationModelFactory")]
+    public static partial class KeyVaultAdministrationModelFactory


Giving timing, we'll leave it as-is for now. If there's concern, we still have a little time to change it.

azure-sdk and others added 2 commits May 24, 2021 14:12

Update AutoRest C# version to 3.0.0-beta.20210524.1

7d277c2

Fix names and namespaces

aa2ad32

ghost added Event Grid IoT KeyVault Search Storage Storage Service (Queues, Blobs, Files) Tables labels May 24, 2021

AlexanderSher mentioned this pull request May 24, 2021

Update AutoRest C# version #21225

Closed

More fixes + API update

9cf7d7d

AlexanderSher requested a review from KrzysztofCwalina as a code owner May 24, 2021 20:56

AlexanderSher added 2 commits May 24, 2021 16:02

Fix comment

ed4741d

API Fix

61f299c

JoshLove-msft approved these changes May 24, 2021

View reviewed changes

Fix FormRecognizer test

9bd143c

maririos reviewed May 24, 2021

View reviewed changes

heaths requested changes May 24, 2021

View reviewed changes

chamons mentioned this pull request May 25, 2021

Update AutoRest C# version #21315

Merged

AlexanderSher added 2 commits May 25, 2021 13:04

Remove CreateUserAndTokenWithNullScopeShouldThrow cause CreateUserAnd…

241be9b

…Token doesn't throw NRE anymore.

Build fix

1b49a5d

annelo-msft reviewed May 25, 2021

View reviewed changes

annelo-msft approved these changes May 25, 2021

View reviewed changes

AlexanderSher added 2 commits May 26, 2021 18:04

- Update package version

8a307b8

- Remove unwanted model factories

Update public API

1159a65

heaths reviewed May 26, 2021

View reviewed changes

Restore some changes to public API

ec48a60

heaths approved these changes May 27, 2021

View reviewed changes

AlexanderSher merged commit 9195ffe into Azure:master May 27, 2021

AlexanderSher deleted the AddModelFactory branch May 27, 2021 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add autorest-generated model factories #21301

Add autorest-generated model factories #21301

AlexanderSher commented May 24, 2021 •

edited by annelo-msft

Loading

JoshLove-msft left a comment

maririos May 24, 2021

AlexanderSher May 25, 2021

heaths left a comment

heaths May 24, 2021

AlexanderSher May 26, 2021

heaths May 26, 2021

heaths May 27, 2021

AlexanderSher commented May 25, 2021

annelo-msft May 25, 2021

AlexanderSher May 26, 2021

annelo-msft left a comment

heaths commented May 25, 2021

AlexanderSher commented May 26, 2021

heaths May 26, 2021

heaths May 27, 2021

Add autorest-generated model factories #21301

Add autorest-generated model factories #21301

Conversation

AlexanderSher commented May 24, 2021 • edited by annelo-msft Loading

JoshLove-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heaths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderSher commented May 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annelo-msft left a comment

Choose a reason for hiding this comment

heaths commented May 25, 2021

AlexanderSher commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderSher commented May 24, 2021 •

edited by annelo-msft

Loading