-
Notifications
You must be signed in to change notification settings - Fork 736
Code changes to generate resources/subresources #1769
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
Changes from 3 commits
48f1173
4b7d7d7
9e3e93b
0858bed
0cc3bea
3f857a1
9f5c1f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,20 +61,22 @@ public override async Task Generate(CodeModel cm) | |
| foreach (CompositeTypeRba model in codeModel.ModelTypes) | ||
| { | ||
| if ((model.Extensions.ContainsKey(AzureExtensions.ExternalExtension) && | ||
| (bool)model.Extensions[AzureExtensions.ExternalExtension]) | ||
| || model.Name == "Resource" || model.Name == "SubResource") | ||
| (bool)model.Extensions[AzureExtensions.ExternalExtension])) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| if( codeModel.pageModels.Any( each => each.Name.EqualsIgnoreCase(model.Name ) ) ) | ||
| if ( codeModel.pageModels.Any( each => each.Name.EqualsIgnoreCase(model.Name ) ) ) | ||
| { | ||
| // Skip, handled in the .pageModels section below. | ||
| continue; | ||
| } | ||
|
|
||
| var modelTemplate = new ModelTemplate { Model = model }; | ||
| await Write(modelTemplate, Path.Combine(GeneratorSettingsRb.Instance.modelsPath, CodeNamer.UnderscoreCase(model.Name) + ImplementationFileExtension)); | ||
| var modelTemplate = new AzureModelTemplate { Model = model }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking into AzureModelTemplate the only difference I saw with Model template is an if statement asking if an accessor should be generated and avoid generating it if the model is Resource or Subsresource, is this correct? If so, could we be making the distinction here and producing the template with the accessor (AzureModelTemplate) if the model matches a resource or subsresource, and produce a Model template in the rest of the cases?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed offline, since we have to create AzureModelTemplate anyway, it is good to keep the Azure specific model generation in one place than two. I am not convinced that we need to continue to use 2 file. No changes here
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I liked the idea that @veronicagg has but looks like you guys already discussed and reached to mutual conclusion |
||
| if (!CompositeTypeRba.resourceOrSubResourceRegEx.IsMatch(model.Name.ToString()) || !CompositeTypeRba.IsResourceModelMatchingStandardDefinition(model)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not 100% sure so do we really need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was missed in my previous change. You do not need to convert it to string. Changed it. |
||
| { | ||
| await Write(modelTemplate, Path.Combine(GeneratorSettingsRb.Instance.modelsPath, CodeNamer.UnderscoreCase(model.Name) + ImplementationFileExtension)); | ||
| } | ||
| } | ||
| // Paged Models | ||
| foreach (var pageModel in codeModel.pageModels) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,9 +1,13 @@ | ||||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||||
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||||
|
|
||||
| using System; | ||||
| using System.Linq; | ||||
| using System.Collections.Generic; | ||||
| using AutoRest.Extensions.Azure; | ||||
| using AutoRest.Ruby.Model; | ||||
| using AutoRest.Core.Model; | ||||
| using System.Text.RegularExpressions; | ||||
|
|
||||
| namespace AutoRest.Ruby.Azure.Model | ||||
| { | ||||
|
|
@@ -12,6 +16,10 @@ namespace AutoRest.Ruby.Azure.Model | |||
| /// </summary> | ||||
| public class CompositeTypeRba : CompositeTypeRb | ||||
| { | ||||
| public static readonly Regex resourceOrSubResourceRegEx = new Regex(@"^(RESOURCE|SUBRESOURCE)$", RegexOptions.IgnoreCase); | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another minor: why are you formatting all = with vertical alignment? Again not a standard I've seen so for. Example:
I don't want to be a blocker here but I like consistency so if you find too many minor comments please bear with me :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed it... Just a personal choice. But, will change it hereafter |
||||
| private static readonly Regex subResourceRegEx = new Regex(@"^(ID)$", RegexOptions.IgnoreCase); | ||||
| private static readonly Regex resourceRegEx = new Regex(@"^(ID|NAME|TYPE|LOCATION|TAGS)$", RegexOptions.IgnoreCase); | ||||
|
|
||||
| /// <summary> | ||||
| /// Initializes a new instance of the AzureModelTemplateModel class. | ||||
| /// </summary> | ||||
|
|
@@ -42,15 +50,66 @@ public override string GetBaseTypeName() | |||
| if (this.BaseModelType.Extensions.ContainsKey(AzureExtensions.ExternalExtension) || | ||||
| this.BaseModelType.Extensions.ContainsKey(AzureExtensions.AzureResourceExtension)) | ||||
| { | ||||
| typeName = "MsRestAzure::" + typeName; | ||||
| if (!resourceOrSubResourceRegEx.IsMatch(typeName) || !IsResourceModelMatchingStandardDefinition(this)) | ||||
| { | ||||
| typeName = "MsRestAzure::" + typeName; | ||||
| } | ||||
| } | ||||
|
|
||||
| return " < " + typeName; | ||||
| } | ||||
| else if (resourceOrSubResourceRegEx.IsMatch(this.Name)) | ||||
| { | ||||
| return " < " + "MsRestAzure::" + this.Name; | ||||
| } | ||||
|
|
||||
| return string.Empty; | ||||
| } | ||||
|
|
||||
| /// <summary> | ||||
| /// Checks if the provided definition of models 'Resource'/'SubResource' matches the standard definition. | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you may want to clarify what standard is here, "as defined in MsRestAzure"
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should the description look something like this: "Checks if the provided definition of model, matches the definition of 'Resource'/'SubResource' as defined in MsRestAzure" ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added the clarification |
||||
| /// For other models, it returns false. | ||||
| /// </summary> | ||||
| /// <param name="model">to be validated</param> | ||||
| /// <returns></returns> | ||||
| public static bool IsResourceModelMatchingStandardDefinition(CompositeType model) | ||||
| { | ||||
| string modelName = model.Name.ToString(); | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since I am using this in string comparison with ignore case, this needs to be changed to string |
||||
| if (modelName.Equals("SubResource", StringComparison.InvariantCultureIgnoreCase) && | ||||
| model.Properties.All(property => subResourceRegEx.IsMatch(property.Name.ToString()))) | ||||
| { | ||||
| return true; | ||||
| } | ||||
|
|
||||
| if(modelName.Equals("Resource", StringComparison.InvariantCultureIgnoreCase) && | ||||
| model.Properties.All(property => resourceRegEx.IsMatch(property.Name.ToString()))) | ||||
| { | ||||
| return true; | ||||
| } | ||||
|
|
||||
| return false; | ||||
| } | ||||
|
|
||||
| /// <summary> | ||||
| /// Determines if the accessor needs to be generated. For Resource/SubResource models, accessors are generated only | ||||
| /// for properties that are not in the standard definition. | ||||
| /// </summary> | ||||
| /// <param name="model"></param> | ||||
| /// <param name="propertyName"></param> | ||||
| /// <returns></returns> | ||||
| public static bool NeedsAccessor(CompositeType model, string propertyName) | ||||
| { | ||||
| string modelName = model.Name.ToString(); | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not 100% sure so do we really need ToString() in model.Name.ToString()?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am 100% sure. Checked with the debugger also. You need it here. |
||||
| if((modelName.Equals("SubResource", StringComparison.InvariantCultureIgnoreCase) && subResourceRegEx.IsMatch(propertyName)) || | ||||
| (modelName.Equals("Resource", StringComparison.InvariantCultureIgnoreCase) && resourceRegEx.IsMatch(propertyName))) | ||||
| { | ||||
| return false; | ||||
| } | ||||
|
|
||||
| return true; | ||||
| } | ||||
|
|
||||
|
|
||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: extra line
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed it |
||||
| /// <summary> | ||||
| /// Gets the list of modules/classes which need to be included. | ||||
| /// </summary> | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| @using System | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The entire file seems like exact copy of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looked into other options. For now, since we are using cshtml files, we do not have anyother option. No changes here
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay then |
||
| @using System.Linq | ||
| @using AutoRest.Core.Utilities | ||
| @using AutoRest.Ruby | ||
| @using AutoRest.Ruby.Model | ||
| @using AutoRest.Ruby.Azure.Model | ||
| @inherits AutoRest.Core.Template<AutoRest.Ruby.Model.CompositeTypeRb> | ||
| # encoding: utf-8 | ||
| @Header("# ") | ||
| @EmptyLine | ||
| module @(Settings.Namespace) | ||
| module Models | ||
| # | ||
| @WrapComment("# ", Model.BuildSummaryAndDescriptionString()) | ||
| # | ||
| class @Model.Name@(Model.GetBaseTypeName()) | ||
| @if (Model.Includes.Any()) | ||
| { | ||
| @EmptyLine | ||
| foreach (var include in Model.Includes) | ||
| { | ||
| @:include @include | ||
| } | ||
| @EmptyLine | ||
| } | ||
|
|
||
| @if (Model.BaseIsPolymorphic && Model.BaseModelType == null) | ||
| { | ||
| @:@@@@discriminatorMap = Hash.new | ||
| foreach (var derivedType in Model.DerivedTypes) | ||
| { | ||
| @:@@@@discriminatorMap["@derivedType.SerializedName"] = "@derivedType.Name" | ||
| } | ||
| } | ||
|
|
||
| @if (Model.BaseIsPolymorphic) | ||
| { | ||
| @EmptyLine | ||
| @:def initialize | ||
| @: @@@Model.PolymorphicDiscriminatorProperty.Name = "@Model.SerializedName" | ||
| @:end | ||
| @EmptyLine | ||
| @:attr_accessor :@Model.PolymorphicDiscriminatorProperty.Name | ||
| @EmptyLine | ||
| } | ||
|
|
||
| @foreach (var property in Model.PropertyTemplateModels.Where(each => !each.IsPolymorphicDiscriminator)) | ||
| { | ||
| @if (CompositeTypeRba.NeedsAccessor(Model, property.Name)) | ||
| { | ||
| @:@WrapComment("# ", string.Format("@return {0}{1}", property.ModelType.GetYardDocumentation(), CompositeTypeRb.GetPropertyDocumentationString(property))) | ||
| @:attr_accessor :@property.Name | ||
| @EmptyLine | ||
| @: | ||
| } | ||
| } | ||
|
|
||
| @EmptyLine | ||
| # | ||
| @WrapComment("# ", string.Format("Mapper for {0} class as Ruby Hash.", Model.Name)) | ||
| # This will be used for serialization/deserialization. | ||
| # | ||
| def self.mapper() | ||
| @(Model.ConstructModelMapper()) | ||
| end | ||
| end | ||
| end | ||
| end | ||
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.
sorry for the nit-pick here but I don't think this format is the standard we have followed at-least in this code base.
just one space after
ifand(. No spaces bertween(&codeModelor between(&)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.
Moreover, even you haven't followed the same across your changes as well, so looks like minor formatting error :)
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.
Done