Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Added SubResource/Resource Hierarchy #663

Merged
sarangan12 merged 2 commits into from
Feb 2, 2017
Merged

Added SubResource/Resource Hierarchy #663

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36096bd
Added SubResource/Resource Hierarchy
sarangan12 Jan 30, 2017
46e546b
Additional changes
sarangan12 Jan 31, 2017
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
7 changes: 3 additions & 4 deletions runtime/ms_rest_azure/lib/ms_rest_azure/resource.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See License.txt in the project root for license information.

require_relative 'sub_resource'

module MsRestAzure
#
# Class which represents any Azure resource.
#
class Resource

# @return [String] the id of the resource.
attr_accessor :id
class Resource < SubResource

@vishrutshah vishrutshah Jan 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@sarangan12 sarangan12 Jan 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This needs change in the order in another file. It has been done with the latest commit

@sarangan12 sarangan12 Jan 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Regarding renaming, I think it has some more items to discuss. Let me create a WI and add it to the backlog #664

@veronicagg veronicagg Jan 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@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?

@sarangan12 sarangan12 Jan 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@veronicagg Added detailed context

@vishrutshah vishrutshah Feb 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@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.


# @return [String] the name of the resource.
attr_accessor :name
Expand Down