Update Autorest C# #11305

pakrym · 2020-04-14T17:06:54Z

Minor naming and serialization changes.
Additional CodeGenMember attributes:

Initialize - to always initialize members see https://github.com/Azure/autorest.csharp/blob/feature/v3/test/TestProjects/SerializationCustomization/AlwaysInitializeTestClass.cs#L11
EmptyAsUndefined to avoid serializing empty collections https://github.com/Azure/autorest.csharp/blob/feature/v3/test/TestProjects/SerializationCustomization/EmptyAsUndefinedTestModel.cs#L11

Bug fix for FormRecognizer overloads.
Time type support.

pakrym · 2020-04-14T17:37:44Z

eng/CodeGeneration.targets

    <_SupportsCodeGeneration Condition="'$(IsClientLibrary)' == 'true'">true</_SupportsCodeGeneration>
    <_DefaultInputName Condition="Exists('$(MSBuildProjectDirectory)/autorest.md')">$(MSBuildProjectDirectory)/autorest.md</_DefaultInputName>
    <AutorestInput Condition="'$(AutorestInput)' == ''">$(_DefaultInputName)</AutorestInput>
-    <AutorestConfiguration Condition="'$(AutorestConfiguration)' == '' AND '$(AutorestInput)' != '$(_DefaultInputName)'">$(_DefaultInputName)</AutorestConfiguration>


We don't need this feature. Autorest now supports including remote config into a local one.

pakrym · 2020-04-14T17:48:53Z

sdk/search/Azure.Search.Documents/src/Models/SynonymMap.cs


 namespace Azure.Search.Documents.Models
 {
+    [CodeGenSuppress(nameof(SynonymMap), typeof(string), typeof(string))]


We don't suppress generated ctors any more if you have a custom one so this have to be done manually.

heaths

Sampling of Search and general changes LGTM, though a couple of questions.

heaths · 2020-04-14T17:37:55Z

sdk/core/Azure.Core/src/Shared/Autorest/ManagementPipelineBuilder.cs

+    {
+        public static HttpPipeline Build(TokenCredential credential, ClientOptions options)
+        {
+            return HttpPipelineBuilder.Build(options, new BearerTokenAuthenticationPolicy(credential, "https://management.azure.com//.default"));


Probably not a big deal since they usually get canonicalized, but did you mean for two "//" here toward the end of the URI?

Yeah, actually @schaabs told me to do it :)

It's not technically a URI it's a scope string, which has to be exact. Since the resource string registered with AAD, "https://management.azure.com/", ends in a "/" the default scope has the two "//", "https://management.azure.com//.default". It's unfortunate because it's inconsistent between services, and is dependent on how the resource was registered with AAD. For instance the resource for key vault is "https://vaults.azure.net" doesn't end in a "/" so its default scope, "https://vaults.azure.net/.default" does not have the double slashes.

heaths · 2020-04-14T17:49:11Z

sdk/search/Azure.Search.Documents/src/Generated/Models/AnalyzerName.cs

        public override bool Equals(object obj) => obj is AnalyzerName other && Equals(other);
        /// <inheritdoc />
-        public bool Equals(AnalyzerName other) => string.Equals(_value, other._value, StringComparison.Ordinal);
+        public bool Equals(AnalyzerName other) => string.Equals(_value, other._value, StringComparison.InvariantCultureIgnoreCase);


Were the extensible enum struct guidelines updated? This is a change from them. It was intentional, but open for change.

Guidelines don't define any comparison. There is a sample that uses Ordinal but it's not explicitly mandated. https://azure.github.io/azure-sdk/dotnet_implementation.html#dotnet-enums

I know - I wrote that after discussing the finer points with the team. :) We should be consistent across implementations. If we're going to accept case-insensitive parsing, we should do so at least by default across implementations.

@heaths @pakrym The service decides whether these things are case-insensitive or not, not the client. I don't think it's possible to have a generated implementation that works for the general case.

maririos · 2020-04-14T19:51:04Z

Thanks Pavel!

tg-msft

Looks good.

Also, if anyone knows how to tell linguist to expand everything in the github PR view, that would be a nifty trick for these PRs where I want to just keep scrolling.

tg-msft · 2020-04-14T19:57:20Z

sdk/search/Azure.Search.Documents/src/Generated/Models/AnalyzeResult.Serialization.cs

-                        array.Add(TokenInfo.DeserializeTokenInfo(item));
+                        if (item.ValueKind == JsonValueKind.Null)
+                        {
+                            array.Add(null);
+                        }
+                        else
+                        {
+                            array.Add(TokenInfo.DeserializeTokenInfo(item));
+                        }


I don't have a problem with this - but any reason not to pass it into the DeserializeTokenInfo method and let it handle how it chooses to interpret null? If we're going to make it possible to customize Deserialize methods down the road (if we don't already), it might be nice to also customize how null gets handled.

Interesting idea I haven't considered doing it this way yet. Not sure I want to change it before we have use cases but I'll keep it mind.

pakrym · 2020-04-14T20:34:37Z

Also, if anyone knows how to tell linguist to expand everything in the github PR view, that would be a nifty trick for these PRs where I want to just keep scrolling.

Using Refined Github extension you can alt+click on Load diff text and all would get expanded.

heaths · 2020-04-14T20:49:41Z

I have on my personal backlog to push a PR to github.vscode-pull-request-github to allow it to handle system URI monikers (Code supports this) and then write a simple browser extension to add an "Open in Code" button, similar to what AzDO has for VS. But if someone got to this first...cool.

pakrym · 2020-04-14T20:51:41Z

If only VS code Github review didn't have 300 file count limit..

heaths · 2020-04-14T21:02:33Z

Is that only if reviewing the PR itself vs. checking out the branch? I thought the latter was possible for larger PRs (and provides more context).

pakrym · 2020-04-14T21:22:03Z

Only for PR review feature. For checkout it uses git so that works fine.

pakrym added 3 commits April 14, 2020 09:49

Update Autorest C#

a5af404

More

62ad993

Shared source

6a03354

pakrym requested review from heaths and tg-msft and removed request for tg-msft April 14, 2020 17:34

pakrym commented Apr 14, 2020

View reviewed changes

pakrym marked this pull request as ready for review April 14, 2020 17:40

pakrym requested review from AlexGhiondea, KrzysztofCwalina, arv100kri, bleroy, brjohnstmsft, chidozieononiwu, danieljurek, mitchdenny and weshaggard as code owners April 14, 2020 17:40

pakrym commented Apr 14, 2020

View reviewed changes

heaths approved these changes Apr 14, 2020

View reviewed changes

tg-msft approved these changes Apr 14, 2020

View reviewed changes

pakrym merged commit 72e0029 into Azure:master Apr 14, 2020

pakrym deleted the pakrym/update-generator-14 branch April 14, 2020 20:36

Update Autorest C# #11305

Update Autorest C# #11305

Uh oh!

Conversation

pakrym commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heaths left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schaabs Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maririos commented Apr 14, 2020

Uh oh!

tg-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakrym commented Apr 14, 2020

Uh oh!

heaths commented Apr 14, 2020

Uh oh!

pakrym commented Apr 14, 2020

Uh oh!

heaths commented Apr 14, 2020

Uh oh!

pakrym commented Apr 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pakrym commented Apr 14, 2020 •

edited

Loading

schaabs Apr 14, 2020 •

edited

Loading