Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Ruby] Adding resource group accessor to all the resources #1668

Closed
wants to merge 3 commits into from
Closed

[Ruby] Adding resource group accessor to all the resources #1668

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
deb4f81
Adding resource group accessor to all the resources
sarangan12 Dec 7, 2016
fb54290
Changes to bring resource group to MethodRba
sarangan12 Dec 14, 2016
db41529
Regenerated Samples
sarangan12 Dec 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/generator/AutoRest.Ruby/Model/MethodRb.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,16 @@ public string GetDeserializationString(IModelType type, string valueReference =
builder.AppendLine("{1} = @client.deserialize(result_mapper, {0}, '{1}')", responseVariable, valueReference);
}

if (type is CompositeType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this added code be only applicable to Azure related methods? If so, should this be part of AutoRest.Ruby.Azure, instead of AutoRest.Ruby?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that the resource group is an azure concept. Thanks for pointing that. I have changed the code with the latest commit

{
var composite = type as CompositeType;
if (composite.BaseModelType != null && composite.BaseModelType.Name == "Resource")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked offline about the hard-coded string "resource_group_name" and I'm not sure of a way of obtaining this as a variable (for it not to be hard-coded), it seems to be a convention for it to be named that way, so it's probably ok.

Though, should these lines be added to the code only if "resource_group_name" is one of the parameters of the method in which we're adding them? (as that's where the value is coming from)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would be in a much bigger trouble if anyone changes this name. Also, this name is actually hardcoded already in Swagger parser. Also, remember the defined? has been added as a safeguard to avoid any issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, though with this question I was wondering if we could avoid writing these lines of code in the file if there's no resource_group_name coming as parameter of the method. For example the following operation would end up with this added line of code, but it really doesn't apply to the method. Operation: https://github.com/Azure/azure-rest-api-specs/blob/master/arm-compute/2016-03-30/swagger/compute.json#L294, which in generated code looks like this https://github.com/Azure/azure-sdk-for-ruby/blob/master/management/azure_mgmt_compute/lib/generated/azure_mgmt_compute/virtual_machine_extension_images.rb#L163
I understand it would still work because the property is not defined, but it's really not applicable.

{
builder.AppendLine("{0}.resource_group = resource_group_name if defined?resource_group_name", valueReference);
builder.AppendLine("{0}", valueReference);
}
}

return builder.ToString();
}

Expand Down