Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix StorageManagers Cross Linkers #14795

Merged
merged 1 commit into from
Apr 19, 2017
Merged

Fix StorageManagers Cross Linkers #14795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions app/models/manageiq/providers/storage_manager/cinder_manager/refresh_parser/cross_linkers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ def self.cross_link(ems, data)
_log.warn "Manager does not have a parent."
return
end
unless data
_log.warn "Manager does not have volumes, snapshots, or volume backups."
Copy link
Member

Choose a reason for hiding this comment

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

We usually require parenthesis around method arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just duplicating the previous code. Are you asking for me to go back and change existing code, or to have the new code be inconsistent with the old code?

return
end

parent_type = parent_manager.class.ems_type
_log.debug "Parent type: #{parent_type}"
Expand Down
4 changes: 4 additions & 0 deletions app/models/manageiq/providers/storage_manager/swift_manager/refresh_parser/cross_linkers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ def self.cross_link(ems, data)
_log.warn "Manager does not have a parent."
return
end
unless data
_log.warn "Manager does not have any object storage."
Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member Author

Choose a reason for hiding this comment

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

And just for perspective, under manageiq/app/models - there are 1373 calls to log methods using "(" and 642 not using them. It is not consistent although there is a 2/1 ratio for.

Copy link
Member

Choose a reason for hiding this comment

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

That's why I made it just a comment, not request changes. We seem to be losing consistency on this one since the rubocop doesn't complain about it.

Copy link
Member

Choose a reason for hiding this comment

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

We seem to be losing consistency on this one since the rubocop doesn't complain about it.

This is troubling since it should be complaining.

Copy link
Member

Choose a reason for hiding this comment

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

@Fryguy See http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Lint/RequireParentheses
They only seem concerned about having parenthesis if there is && or ||. 😢

return
end

parent_type = parent_manager.class.ems_type
_log.debug "Parent type: #{parent_type}"
Expand Down