Conversation
vishrutshah
left a comment
vishrutshah
left a comment
There was a problem hiding this comment.
Changes looks great!
@sarangan12 💯 Thanks for taking care for this issue 💯
As you have exhaustive understanding of the cases, Would you mind please adding unit tests as that'd greatly help us as well and make sure that no one else is breaking it as well in future :)
veronicagg
left a comment
veronicagg
left a comment
There was a problem hiding this comment.
minor comment, rest looks good, thanks!
Agree with Vishrut, adding a test would be helpful.
Should we remove the "Resource" and "Subresource" definitions from MsRestAzure?
src/azure/Model/CompositeTypeRba.cs
Outdated
| @@ -48,68 +48,12 @@ public override string GetBaseTypeName() | |||
| { | |||
| string typeName = this.BaseModelType.Name; | |||
There was a problem hiding this comment.
should we not declare this variable now, and just return " < " + this.BaseModelType.Name; ? else return string.Empty?
There was a problem hiding this comment.
@veronicagg Yes. the 'typeName' is no longer required. I have removed with the latest commit.
|
and thanks for the extensive testing on this! |
sarangan12
changed the title
|
@vishrutshah @veronicagg I have added the testcases around the inheritance logic. |
|
@veronicagg Just to be on the safe side, I have created issue Azure/azure-sdk-for-ruby#1010 and assigned it to next-arm and myself. I will just wait for one more release before taking that step :-) |
veronicagg
left a comment
veronicagg
left a comment
There was a problem hiding this comment.
couple of minor comments, thanks!
| it 'should generate models with expected inheritance' do | ||
| modules = Module.const_get('AzureResourceInheritanceModule::Models') | ||
|
|
||
| # Should generate InheritMsRestAzureResource with super class as MsRestAzure::Resource |
There was a problem hiding this comment.
I believe this comment is out of date
There was a problem hiding this comment.
Yes. My bad... Fixed it
|
|
||
| include AzureResourceInheritanceModule | ||
|
|
||
| describe 'ResourceInheritance' do |
There was a problem hiding this comment.
should this be renamed, as it's not testing inheritance for resource anymore? but validating that all models are generated for any inheritance relationship?
|
Just adding Issue link here.. Fixing Azure/azure-sdk-for-ruby#957 |
publish jobsuccess (version: 2.0.17) |
4 participants
A swagger file can have 'Resource'/'SubResource' models defined in them.
In the current design, these two models have defined structure in ms_rest_azure. When the provided structure matches with the one defined then the defined ones are used. Else, a new class is defined per gem and this new class derives from the standard ones in MsRestAzure.
But, the above approach does have a problem. When the 'Resource' in swagger has properties such as 'name', 'p1', 'p2', 'p3', the above approach fails.
In order to handle this condition, a new design is provided - 'Generate Resource/SubResource per gem'.
What are the changes included in this PR?
How did I test?
@vishrutshah @veronicagg @salameer Please review and approve,