[Ruby] Adding resource group accessor to all the resources #1668
Conversation
|
Can one of the admins verify this patch? |
|
Hi @sarangan12, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
sarangan12
changed the title
sarangan12
force-pushed
the
AddResourceGroupToResources
branch
3 times, most recently
from
9810e67
to
99d335c
Compare
sarangan12
force-pushed
the
AddResourceGroupToResources
branch
from
99d335c
to
deb4f81
Compare
sarangan12
changed the title
| if (type is CompositeType) | ||
| { | ||
| var composite = type as CompositeType; | ||
| if (composite.BaseModelType != null && composite.BaseModelType.Name == "Resource") |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Agreed that the resource group is an azure concept. Thanks for pointing that. I have changed the code with the latest commit
|
Also, please regenerate AutoRest samples with the change. |
|
@veronicagg Regenerated samples using gulp regenerate:expected:samples command |
veronicagg
changed the title
| { | ||
| var builder = new IndentedStringBuilder(" "); | ||
|
|
||
| string deserializedString = base.GetDeserializationString(type, valueReference, responseVariable); |
There was a problem hiding this comment.
should we be calling super(type, valueReference, responseVariable) ? is there a difference in doing super vs base.method?
There was a problem hiding this comment.
base is used in C#. not super
| if (type is CompositeType) | ||
| { | ||
| var composite = type as CompositeType; | ||
| if (composite.BaseModelType != null && composite.BaseModelType.Name == "Resource") |
There was a problem hiding this comment.
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.
|
@azuresdkci retest this please |
|
This PR is no longer required. Closing this now. |
Related to Azure/azure-sdk-for-ruby#340
@veronicagg @vishrutshah @salameer Please review.