[Go]Paging tests and general paging fixes #1489

mcardosos · 2016-10-04T22:55:07Z

Paging now doesn't duplicate "NextLink" on structs.
Also, it only generates a "next" method when it is not already defined in swagger. This includes not generating a nextLinkPreparer when it is not needed.

…ls file This also causes that if there is already a next method defined in swagger for that model (including generic next methods), autorest wont generate the specific next links.

azuresdkci · 2016-10-04T22:55:09Z

Can one of the admins verify this patch?

azurecla · 2016-10-04T22:55:25Z

Hi @mcardosos, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

It looks like you're working at Microsoft (mcardoso). If you're full-time, we DON'T require a contribution license agreement.

If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

mcardosos · 2016-10-05T01:52:57Z

@jhendrixMSFT @vishrutshah @garimakhulbe
Please take a look :D

vishrutshah · 2016-10-07T22:37:16Z

.gitignore

+src/generator/AutoRest.Go.Azure.Tests/src/github.com/*
+src/generator/AutoRest.Go.Azure.Tests/src/tests/generated/*
+src/generator/AutoRest.Go.Azure.Tests/src/tests/vendor/*
+src/generator/AutoRest.Go.Azure.Tests/src/tests/glide.lock


minor: add a new line character in the last line to not show-up red mark.

vishrutshah · 2016-10-07T22:37:34Z

.gitignore

 src/generator/AutoRest.Go.Tests/src/tests/generated/*
 src/generator/AutoRest.Go.Tests/src/tests/vendor/*
 src/generator/AutoRest.Go.Tests/src/tests/glide.lock
+
+


minor: two empty line is not required.

vishrutshah · 2016-10-07T22:41:04Z

CodeGenerator.sln

@@ -37,6 +37,10 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "AutoRest.Ruby", "src\genera
 EndProject
 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "AutoRest.Ruby.Azure", "src\generator\AutoRest.Ruby.Azure\AutoRest.Ruby.Azure.csproj", "{31931998-7543-41DA-9E58-D9670D810352}"
 EndProject
+Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "AutoRest.Go", "src\generator\AutoRest.Go\AutoRest.Go.xproj", "{BEDCF556-6CEA-4EAE-ACE3-8B9EFE0818E5}"


With this changes, should we update https://github.com/Azure/autorest/blob/master/Documentation/building-code.md ?

Ah, yes we should. Install Golang and glide.

vishrutshah · 2016-10-07T22:41:54Z

gulpfile.js

@@ -174,6 +174,11 @@ var rubyAzureMappings = {
  'parameter_grouping':['../../dev/TestServer/swagger/azure-parameter-grouping.json', 'ParameterGroupingModule']
 };

+var goAzureMappings = {
+  'paging':['../../dev/TestServer/swagger/paging.json','paginggroup'],


minor: I'd add a space between , & 'paginggroup'.

vishrutshah · 2016-10-07T22:42:57Z

gulpfile.js

@@ -190,7 +195,8 @@ gulp.task('regenerate:expected', function(cb){
      'regenerate:expected:python',
      'regenerate:expected:pythonazure',
      'regenerate:expected:samples',
-      'regenerate:expected:go'
+      'regenerate:expected:go',
+      'regenerate:expected:goazure'


If we currently do not have separation between Go & Azure.Go I'd suggest not making it anywhere.

Will move all test to test:go

vishrutshah · 2016-10-07T22:56:53Z

src/generator/AutoRest.Go/TemplateModels/MethodTemplateModel.cs

@@ -22,14 +22,16 @@ public class MethodTemplateModel : Method
        private readonly string lroDescription = " This method may poll for completion. Polling can be canceled by passing the cancel channel argument. " +
                                                 "The channel will be used to cancel polling and any outstanding HTTP requests.";

+        public readonly bool NextAlreadyDefined;


You may want to default it to false

vishrutshah · 2016-10-07T22:57:23Z

src/generator/AutoRest.Go/TemplateModels/MethodTemplateModel.cs

-                       p => p != null && p.IsMethodArgument() && !string.IsNullOrWhiteSpace(p.Name) && !p.SerializedName.IsApiVersion())
-                        .OrderBy(item => !item.IsRequired);
+                        p => p != null && p.IsMethodArgument() && !string.IsNullOrWhiteSpace(p.Name))        
+


minor: empty line in-between code?

vishrutshah · 2016-10-07T22:57:55Z

src/generator/AutoRest.Go/TemplateModels/ModelTemplateModel.cs

@@ -21,6 +21,8 @@ public class ModelTemplateModel : CompositeType
        // (null or empty if the model is not paged).
        public string NextLink;

+        public bool PreparerNeeded;


Should we default to true ?

To false I think. The general case is that models won't need a preparer (I explaned this better in the NextMethodUndefined comment).

vishrutshah · 2016-10-07T22:59:26Z

src/generator/AutoRest.Go/TemplateModels/ModelsTemplateModel.cs

@@ -15,6 +16,7 @@ public class ModelsTemplateModel : ServiceClient
        public List<ModelTemplateModel> ModelTemplateModels { get; set; }
        public string PackageName { get; set; }
        public Dictionary<IType, string> PagedTypes { get; set; }
+        public List<IType> NextMethodUndefined { get; set; }


Could you please elaborate what does this variable represents?

I added this for paged methods that its next method isn't defined in the swagger. How the generator handles this right now is adding a preparer in the models file, specific for the paged model. I am using this list to keep track of which models would need the preparer, so it is not generated when the next method is already defined. When the next method is defined in the swagger, the generator takes it as any other method and generates its preparer, sender and receiver next to the method and the other group methods.

vishrutshah · 2016-10-08T00:38:42Z

src/generator/AutoRest.Go/rakefile.rb

@@ -9,17 +9,18 @@
 AUTOREST     = "../../core/AutoRest/bin/Debug/net451/win7-x64/AutoRest.exe"

 SWAGGER_VERSIONS = {
+	# asazure: {version: "2016-05-06"},


Are you planning to regen the once that you updated in this Rakefile?

I could regen, yes. I updated the rake file just to keep it updated, and add comments on what we are missing.

vishrutshah · 2016-10-12T22:53:03Z

src/generator/AutoRest.Go/GoCodeNamer.cs

@@ -533,6 +533,21 @@ private IType NormalizeDictionaryType(DictionaryType dictionaryType)
            return new MapType(NormalizeTypeReference(dictionaryType.ValueType));
        }

+        public static string PascalCaseWithoutChar(string name, char splitter)


minor: should a comments

jhendrixMSFT · 2016-10-13T19:42:12Z

src/generator/AutoRest.Go/Extensions.cs

@@ -560,7 +562,13 @@ public static string Fields(this CompositeType compositeType)
                &&  !String.IsNullOrEmpty((compositeType as ModelTemplateModel).NextLink))
            {
                var nextLinkField = (compositeType as ModelTemplateModel).NextLink;
-                if (!properties.Any(p => p.Name == nextLinkField))
+                foreach (Property p in properties) {


Can you add a comment explaining why this is necessary.

This foreach removes all dots in properties (for code to work), and if it finds the already existing next link, changes its name to how NextLink was added originally (NextLink is a ModelTemplateModel property and it is used in more places, including the razor tamplates)

mcardosos · 2016-10-17T18:12:04Z

@fearthecowboy Hello :D
Will merging this to master cause problems with refactoring?

fearthecowboy · 2016-10-17T22:54:52Z

LGTM 👍

* Adding validation rules for subscriptionId/api-version params * Introduced Plugin Model (refactored plugin classes) * [Python] CloudError updates (#1510) * Updated CloudError * Added exception documentation * Regenerated tests * All tests working * [Go] Paging tests and general paging fixes (#1489) * Trying to delete duplication * Graph doesn't duplicate odatanextlink * OdataNextLink not duplicated and correctly named * Not needed preparers for paged results are no longer included in models file. This also causes that if there is already a next method defined in swagger for that model (including generic next methods), autorest wont generate the specific next links. * Added paging tests (except fragment link) * Added fragment next link test * Fixed code about preparer for pages types * msrestazure 0.4.4 (#1514) * Addressed PR comments * Remove Python msrest/msrestazure from Autorest repo (#1518) * Remove msrest from Autorest repo * Remove Python msrest from gulpfile * Remove hack in msrestazure to read ../msrest * Update dependencies of Python msrestazure to get msrest from PyPI * Adding mock in msrestazure tests * Simplify tox config for msrestazure * msrestazure 0.4.4 (#1514) * Remove msrestazure Python * Minor code cleanup * Fixing json descriptions * Add min length and max length support to schema generator (#1503) * Add ServiceBus AzureResourceSchema acceptance test * Add minLength and maxLength support to schema generator * Fixing logic to add missing schema resources * Add sql schema test (#1486) * Add ServiceBus AzureResourceSchema acceptance test * Add SQL schema test and fix NotificationHubs schema test * Add service fabric schema tests (#1485) * Add ServiceBus AzureResourceSchema acceptance test * Add ServiceFabric schema test, fix NotificationHubs schema test * Updates/cleanups to plugin model,core and node/c# * small updates to modeler classes * updated project.json files * fixing tests * fixed loader test * Bumping version to fix nuget problem

* Trying to delete duplication * Graph doesn't duplicate odatanextlink * OdataNextLink not duplicated and correctly named * Not needed preparers for paged results are no longer included in models file. This also causes that if there is already a next method defined in swagger for that model (including generic next methods), autorest wont generate the specific next links. * Added paging tests (except fragment link) * Added fragment next link test * Fixed code about preparer for pages types

@anna

* Refactor code model (#1444) * CodeModel Refactoring - Moved/Renamed Files * CodeModel Refactoring - Changes to AutoRest/AutoRest.Core * CodeModel Refactoring - Core Tests changes * CodeModel Refactoring - AutoRest.Extensions changes * CodeModel Refactoring - AutoRest.Extensions Tests changes * CodeModel Refactoring - Fix compiler warnign * CodeModel Refactoring - CSharp Generator * CodeModel Refactoring - CSharp Azure Generator * CodeModel Refactoring - CSharp Tests Changes * CodeModel Refactoring - NodeJS changes * CodeModel Refactoring - NodeJS Azure changes * CodeModel Refactoring - Modeler changes * Fix/enable all tests * Fix up c# unit tests * small fixes in Moder * Some last-minute tweaks to the generators to pass tests * Changes to finish getting all the tests to run (in core) * Extensions tests cleanup * Resharper settings * Tweaking the build to build and test ok via gulp * accidentally overwrote fix for in memory assemblies * Fixed glitch in builing * Tweaking build for linux * cleaning up small issues and removing superflous junk * cleaned up stuff around nullability (and some other minor cleanups) * cleaned up stuff around nullability (and some other minor cleanups) - c# * Regenerated Tests Expected files * small code cleanups * Adding compare script to validate against rest-api-specs repo * added report generation to compare script (requires Araxis Merge installed and configured) * fixed use of clientproperty == null (should use !IsClientProperty) * Introduced Plugin Model (refactored plugin classes) (#1509) * Adding validation rules for subscriptionId/api-version params * Introduced Plugin Model (refactored plugin classes) * [Python] CloudError updates (#1510) * Updated CloudError * Added exception documentation * Regenerated tests * All tests working * [Go] Paging tests and general paging fixes (#1489) * Trying to delete duplication * Graph doesn't duplicate odatanextlink * OdataNextLink not duplicated and correctly named * Not needed preparers for paged results are no longer included in models file. This also causes that if there is already a next method defined in swagger for that model (including generic next methods), autorest wont generate the specific next links. * Added paging tests (except fragment link) * Added fragment next link test * Fixed code about preparer for pages types * msrestazure 0.4.4 (#1514) * Addressed PR comments * Remove Python msrest/msrestazure from Autorest repo (#1518) * Remove msrest from Autorest repo * Remove Python msrest from gulpfile * Remove hack in msrestazure to read ../msrest * Update dependencies of Python msrestazure to get msrest from PyPI * Adding mock in msrestazure tests * Simplify tox config for msrestazure * msrestazure 0.4.4 (#1514) * Remove msrestazure Python * Minor code cleanup * Fixing json descriptions * Add min length and max length support to schema generator (#1503) * Add ServiceBus AzureResourceSchema acceptance test * Add minLength and maxLength support to schema generator * Fixing logic to add missing schema resources * Add sql schema test (#1486) * Add ServiceBus AzureResourceSchema acceptance test * Add SQL schema test and fix NotificationHubs schema test * Add service fabric schema tests (#1485) * Add ServiceBus AzureResourceSchema acceptance test * Add ServiceFabric schema test, fix NotificationHubs schema test * Updates/cleanups to plugin model,core and node/c# * small updates to modeler classes * updated project.json files * fixing tests * fixed loader test * Bumping version to fix nuget problem * Updated AzureResourceGenerator to work in new code model * Ruby Refactored To New Code Model (#1535) * Ruby Refactored To New Code Model * fixed small glitches in generator * reverted incorrect fix * checkpoint - work in progress * checkpoint - work in progress #2 * checkpoint - work in progress #3 * stop generating tmp files * Upcased Module name for enums * Added ModuleName to EnumTypeRb * Used Rb type EnumType in Plugin * sigh * Deleted missing paren * Update CodeGeneratorRba.cs * Update CodeGeneratorRb.cs * merge from master and regenerate a couple files * Update gulpfile.js * Fixing property names with leading underscores (#1546) Fixing property names with leading underscores * Changes to the Python Code Generator for the Refactored Code Model (#1539) * Checkpoint - Merged RefactorCodeModel and NewPython branches * Checkpoint - Merged RefactorCodeModel and NewPython branches * Missed file * Checkpoint - Python, tracking down last glitches (still have to fix the composite swagger merge problem) * Checkpoint - Python cleaned up some more glitches * Make sure that compositemodeler is using types from the model. * Python Changes, reviewed by @anna and @lmazuel * regenerated tests * fixup node test failure after refactor * fixed overzealous use of capital 'd' Decimal * updated version to 1.0 ( still in preview) * Temporarily removing Go, Java, Fluent projects from solution until stable in new model

mcardosos added 13 commits September 6, 2016 15:38

Trying to delete duplication

4f32f7a

Garima last merge

833a18a

Graph doesnt duplicate odatanextlink

fed81e8

OdataNextLink not duplicated and correctly named

b112e47

Not duplicated Next methods

9d00153

Not needed preparers for paged results are no longer included in mode…

76fb3f8

…ls file This also causes that if there is already a next method defined in swagger for that model (including generic next methods), autorest wont generate the specific next links.

[Go] Added paging tests (except fragment link)

e59d03f

Merged master to graph

ff5f147

[Go]Added fragment next link test

cc99dd5

Fixed code about preparer for pages types

c020616

Merge master branch

7c0c403

Merge branch 'master' of https://github.com/Azure/autorest into graph

b84d208

Cute and ready for PR...

d68400c

azurecla added the cla-not-required label Oct 4, 2016

vishrutshah self-assigned this Oct 7, 2016

vishrutshah suggested changes Oct 8, 2016

View reviewed changes

mcardosos added 2 commits October 11, 2016 14:48

Merge branch 'master' of https://github.com/Azure/autorest into paging

092642b

Moved all go tests to gulp test:go

f41e6b8

vishrutshah approved these changes Oct 12, 2016

View reviewed changes

jhendrixMSFT reviewed Oct 13, 2016

View reviewed changes

Added comments

32e755b

vishrutshah merged commit 87cdf99 into Azure:master Oct 18, 2016

mcardosos deleted the paging branch October 20, 2016 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Go]Paging tests and general paging fixes #1489

[Go]Paging tests and general paging fixes #1489

mcardosos commented Oct 4, 2016

azuresdkci commented Oct 4, 2016

azurecla commented Oct 4, 2016

mcardosos commented Oct 5, 2016

vishrutshah Oct 7, 2016

vishrutshah Oct 7, 2016

vishrutshah Oct 7, 2016

mcardosos Oct 10, 2016

vishrutshah Oct 7, 2016

vishrutshah Oct 7, 2016

mcardosos Oct 10, 2016

vishrutshah Oct 7, 2016

vishrutshah Oct 7, 2016

vishrutshah Oct 7, 2016

mcardosos Oct 10, 2016

vishrutshah Oct 7, 2016

mcardosos Oct 10, 2016

vishrutshah Oct 8, 2016

mcardosos Oct 10, 2016

vishrutshah Oct 12, 2016

jhendrixMSFT Oct 13, 2016

mcardosos Oct 17, 2016

mcardosos commented Oct 17, 2016

fearthecowboy commented Oct 17, 2016

[Go]Paging tests and general paging fixes #1489

[Go]Paging tests and general paging fixes #1489

Conversation

mcardosos commented Oct 4, 2016

azuresdkci commented Oct 4, 2016

azurecla commented Oct 4, 2016

mcardosos commented Oct 5, 2016

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcardosos commented Oct 17, 2016

fearthecowboy commented Oct 17, 2016