Implement discriminator support in v3 generator #113

vladbarosan · 2018-03-22T23:48:01Z

Port various fixes from v2 generator
port discriminator support
implement support in xml
Seperate xml and json marshallers

vladbarosan · 2018-03-22T23:54:33Z

Here is sample genereated from Files swagger :
https://github.com/vladbarosan/azure-storage-files/tree/master/storage/dataplane/2017-04-17/files

Swagger is there also:
https://github.com/vladbarosan/azure-storage-files/blob/master/storage/dataplane/2017-04-17/files.json

vladbarosan · 2018-03-23T00:25:33Z

src/Model/PropertyGo.cs

+        /// <summary>
+        ///  Gets if the property should be treated as a pointer
+        /// </summary>
+        public bool IsPointer => !(this.ModelType.HasInterface() || IsRequired || ModelType.CanBeNull() || ModelType is EnumTypeGo);


EnumTypeGo [](start = 120, length = 10)

in v2 we had the extra condition here to be a named enum to be a value type. But I saw we were not using that in v3. Let me know if we want to add the extra criteria

vladbarosan · 2018-03-23T00:27:04Z

src/CodeGeneratorGo.cs

@@ -17,7 +17,6 @@ public class CodeGeneratorGo : CodeGenerator
        private const string ModelsFileName = "models";
        private const string ClientFileName = "client";
        private const string VersionFileName = "version";
-        private const string MarshallingFileName = "marshalling";


MarshallingFileName [](start = 29, length = 19)

note that I removed the marshalling file from the output. it seems most of it should go to the models file

The reason for this file was to separate the exported types from the non-exported types/implementation details (i.e. the content of marshalling.go is interesting to us but not to consumers of the SDK). If merging this into the models file makes it easier to generate the content then we can leave it merged.

makes sense, but I think to keep the generator code cleaner its easier to put them in the models also would prefer to have all the publicly used types ( and their marshallers ) to be in the models.

vladbarosan · 2018-03-23T00:28:17Z

src/Templates/ModelTemplate.cshtml

+        return d.DecodeElement(@local, &start)
+    }
+    </text>
+}


I think this should go in the marshallingxml template

vladbarosan · 2018-03-23T00:29:23Z

src/Templates/MarshallingXml.cshtml

+@using AutoRest.Core.Model
+@using AutoRest.Core.Utilities
+@using AutoRest.Go
+@using AutoRest.Go.Model


main interest is on the xml so maybe focus on this one

vladbarosan · 2018-03-23T00:30:26Z

Also note that right now for XML we are using the tag names to determine the specific type in polymorphic situations. This is what storage supports right now.

mcardosos · 2018-03-24T00:22:00Z

src/Extensions.cs

+                case PrimaryTypeGo primaryType:
+                    primaryType.AddImports(imports);
+                    break;
+                default:


default [](start = 16, length = 7)

Could a different go type arrive here? Something different to a sequence?

we have it the same in the v2 generator. looking at it, Composite Type also has AddImports but that will be just the imports of its fields

jhendrixMSFT · 2018-03-27T18:30:14Z

src/Templates/EnumTemplate.cshtml

 @EmptyLine
-@if (values.Any())
+@if (Model.Values.Any())


Is the above declaration of values no longer needed?

yes ill remove

olydis · 2018-03-27T21:19:56Z

🤖 AutoRest automatic publish job 🤖

success (version: 3.0.46)

vladbarosan · 2018-03-30T19:25:46Z

for blobs look at changes introduced in the last commit : https://github.com/vladbarosan/azure-storage-blob-go/tree/vladdb

jhendrixMSFT · 2018-03-30T20:02:25Z

src/Model/CodeModelGo.cs

-                return $"Azure-SDK-For-Go/{Version} arm-{Namespace}/{ApiVersion}";
-            }
-        }
+        public string UserAgent => $"Azure-SDK-For-Go/{Version} arm-{Namespace}/{ApiVersion}";


Small tweak unrelated, could you remove that "arm-" prefix?

jhendrixMSFT · 2018-03-30T20:09:24Z

test/src/tests/generated/httpinfrastructure/models.go

@@ -70,18 +70,18 @@ type Error struct {
 }

 // Response returns the raw HTTP response object.
-func (e Error) Response() *http.Response {
-	return e.rawResponse
+func (eVar Error) Response() *http.Response {


Curious why this var name changed? Not a big deal, just wondering.

because we added a couple of reserved names and e was one of them.

jhendrixMSFT

Looks good with just a few comments/questions.

ghost assigned vladbarosan Mar 22, 2018

ghost added the review label Mar 22, 2018

vladbarosan requested review from jhendrixMSFT and mcardosos March 22, 2018 23:48

vladbarosan commented Mar 23, 2018

View reviewed changes

mcardosos reviewed Mar 24, 2018

View reviewed changes

jhendrixMSFT reviewed Mar 27, 2018

View reviewed changes

Vlad Barosan added 7 commits March 29, 2018 09:20

Refactoring and missing properties

c5b1736

make model changes

238fe60

Fix crash error

2fe2c7a

Port discriminator support and various fixes from v2

c8528e0

Implement xml unmarshalling

e22d779

Fix and regenerate tests

415e4e1

Populate discriminator field on unmarshalling

236966b

vladbarosan force-pushed the implementDiscriminator branch 2 times, most recently from 7191b0e to 531d631 Compare March 30, 2018 19:22

Fix enum names and unmarshalling bug

7718176

vladbarosan force-pushed the implementDiscriminator branch from 531d631 to 7718176 Compare March 30, 2018 19:27

Fix property xml tag name

7a5644e

jhendrixMSFT reviewed Mar 30, 2018

View reviewed changes

jhendrixMSFT approved these changes Mar 30, 2018

View reviewed changes

remove arm- prefix from user agent string

8e78dd1

vladbarosan force-pushed the implementDiscriminator branch from 0b1e161 to 8e78dd1 Compare April 2, 2018 15:41

vladbarosan merged commit dfc293c into Azure:v3 Apr 2, 2018

ghost removed the review label Apr 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement discriminator support in v3 generator #113

Implement discriminator support in v3 generator #113

vladbarosan commented Mar 22, 2018

vladbarosan commented Mar 22, 2018 •

edited

Loading

vladbarosan Mar 23, 2018 •

edited

Loading

vladbarosan Mar 23, 2018

jhendrixMSFT Mar 26, 2018

vladbarosan Mar 27, 2018

vladbarosan Mar 23, 2018 •

edited

Loading

vladbarosan Mar 23, 2018

vladbarosan commented Mar 23, 2018

mcardosos Mar 24, 2018

vladbarosan Mar 27, 2018

jhendrixMSFT Mar 27, 2018

vladbarosan Mar 29, 2018

olydis commented Mar 27, 2018 •

edited

Loading

vladbarosan commented Mar 30, 2018

jhendrixMSFT Mar 30, 2018

jhendrixMSFT Mar 30, 2018

vladbarosan Apr 2, 2018

jhendrixMSFT left a comment

Implement discriminator support in v3 generator #113

Implement discriminator support in v3 generator #113

Conversation

vladbarosan commented Mar 22, 2018

vladbarosan commented Mar 22, 2018 • edited Loading

vladbarosan Mar 23, 2018 • edited Loading

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

vladbarosan Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladbarosan commented Mar 23, 2018

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

olydis commented Mar 27, 2018 • edited Loading

🤖 AutoRest automatic publish job 🤖

success (version: 3.0.46)

vladbarosan commented Mar 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhendrixMSFT left a comment

Choose a reason for hiding this comment

vladbarosan commented Mar 22, 2018 •

edited

Loading

vladbarosan Mar 23, 2018 •

edited

Loading

vladbarosan Mar 23, 2018 •

edited

Loading

olydis commented Mar 27, 2018 •

edited

Loading