Added SubResource/Resource Hierarchy #663
Conversation
|
|
||
| # @return [String] the id of the resource. | ||
| attr_accessor :id | ||
| class Resource < SubResource |
There was a problem hiding this comment.
Just curious why not use full namespace while inhering to avoid conflicts like class Resource < MsRestAzure::SubResource. That should also eliminate need for require_relative 'sub_resource'.
Moreover the name Sub_Resource inherited into Resource doesn't make sense as developer. I'd suggest renaming it as you mentioned that SubResource is not at all Azure concept so we better avoid explicitly referring it as SubResource.
There was a problem hiding this comment.
This needs change in the order in another file. It has been done with the latest commit
There was a problem hiding this comment.
Regarding renaming, I think it has some more items to discuss. Let me create a WI and add it to the backlog #664
There was a problem hiding this comment.
@sarangan12 sorry, I'm not sure I understand your first comment here (in reply to Vishrut), can you clarify?
Also, what type of items are your referring to that need discussion for renaming?
There was a problem hiding this comment.
@veronicagg Added detailed context
There was a problem hiding this comment.
@sarangan12 Here is my view, If you think naming sounds perfect today and we are good with that I'd keep the names as-is and not even open backlog items. If you have even a slight concern on naming, I'd like that to be fix with this changes rather than a backlog item.
That's what I think but i'll leave it up to you.
|
Adding @devigned |
|
@sarangan12 could you add some context to the issue related to this PR (and reference it)? for our reference later on it'd be good to describe what problem is being solved, what options were evaluated, what option was picked for implementation and why. |
|
|
||
| # @return [String] the id of the resource. | ||
| attr_accessor :id | ||
| class Resource < SubResource |
There was a problem hiding this comment.
@sarangan12 sorry, I'm not sure I understand your first comment here (in reply to Vishrut), can you clarify?
Also, what type of items are your referring to that need discussion for renaming?
|
|
||
| # @return [String] the id of the resource. | ||
| attr_accessor :id | ||
| class Resource < SubResource |
There was a problem hiding this comment.
@sarangan12 Here is my view, If you think naming sounds perfect today and we are good with that I'd keep the names as-is and not even open backlog items. If you have even a slight concern on naming, I'd like that to be fix with this changes rather than a backlog item.
That's what I think but i'll leave it up to you.
Context for this PR:
In the Swagger Files, there will be a definition for "Resource" and "SubResource" (if they are used). These definitions have historically remained same across the services.
Resource: name, id, type, location, tags
SubResource: id
Now, there are services such as datalakeanalyticslake, datalakeanalyticsservice, recoveryservicesbackup, etc have added additional properties to them (which is valid). For example, Sku has been added to Resources.
How are we going to handle it?
Option 1: We could generate the Resource and SubResource classes per service. This is the approach taken by node sdk. This has been discussed and rejected.
Option 2: We could keep the Resource and SubResource classes as such in the runtime. We could generate the similar classes per service only when they diverge from the standard definition. This is the approach taken by this PR. As an additional enhancement, we have established the relation between the SubResource and Resource classes themselves.
Note: One another suggestion was to rename the class "SubResource" to "Identifiable". This is a valid suggestion. But, this needs additional considerations. Are we going to reflect the namechange in all SDKs? Are we going to use that Identifiable class as parent class for any model that has only the Id property? There may be other questions. I have made a call to not do that in this PR. I have opened a seperate issue #664
Ref #1: #659
Ref #2: #647
Ref #3: Failure of nightly builds
@veronicagg @vishrutshah @salameer Please review.