[In Review] Updating the inheritance logic for Resource & SubResource for azure-arm Ruby generator #9
Conversation
…rm Ruby generator
|
@vishrutshah, |
|
@olydis : QQ Seems like CI is not getting picked up? Any idea? or did i do something :) |
|
ohh got picked up..nevermind ...awesome 👍 |
|
@veronicagg @sarangan12 Feel free to review this when you get a chance. @olydis could you please restart the win32-x64 build when you get a chance. Thanks! |
|
|
|
@sarangan12 @veronicagg Feel free to review this when you get a chance. Thanks! |
publish jobsuccess (version: 2.0.14) |
veronicagg
left a comment
veronicagg
left a comment
There was a problem hiding this comment.
Left a few comments, event though this is now merged, sorry it took me longer to get to this review.
| this.BaseModelType.Extensions.ContainsKey(AzureExtensions.AzureResourceExtension)) | ||
| { | ||
| if (!resourceOrSubResourceRegEx.IsMatch(typeName) || !IsResourceModelMatchingStandardDefinition(this)) | ||
| if (resourceOrSubResourceRegEx.IsMatch(typeName) || IsResourceModelMatchingStandardDefinition(this)) |
There was a problem hiding this comment.
why is this an || and not and &&, I believe the name may not necessarily be enough.
|
|
||
| if(modelName.EqualsIgnoreCase("Resource") && | ||
| model.Properties.All(property => resourceRegEx.IsMatch(property.Name.ToString()))) | ||
| if (model.Properties.All(property => subResourceRegEx.IsMatch(property.Name.ToString())) || |
There was a problem hiding this comment.
are name checks enough for this? I believe "read-only" properties may vary, not sure why, but wonder if it can cause problems... for example: https://github.com/Azure/azure-rest-api-specs/blob/97e69db426af23a0cdfde5e5df748565ff1070b8/specification/network/resource-manager/Microsoft.Network/2017-09-01/network.json#L99 and https://github.com/Azure/azure-rest-api-specs/blob/bc175dbf36fd6d8e4ccac18a3bfd102d43fc6672/specification/compute/resource-manager/Microsoft.Compute/2017-03-30/compute.json#L6828
| expect(azure_resource_additional_properties.is_a?(Class)).to be_truthy | ||
|
|
||
| # Should not generate Resource class | ||
| expect { modules.const_get('Resource') }.to raise_error(NameError) |
There was a problem hiding this comment.
"Resource" class would exist from MsRestAzure due to the declaration at https://github.com/Azure/autorest.ruby/pull/9/files#diff-f1567bd7380ca2e7818c4047e9cdd67dR194 right?
Is this just checking that we did not generate a class withe the name "Resource"?
6 participants
Fixes Azure/azure-sdk-for-ruby#957
We'll inherit from the
MsRestAzure::ResourceorMsRestAzure::SubResource, when either name matches the regular expression or we determine that name matches and model has all properties standard properties.