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.

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

Apply rubocop fixes to RoleManagement #20881

Merged
merged 1 commit into from
Jan 13, 2021
Merged

Apply rubocop fixes to RoleManagement #20881

merged 1 commit into from
Jan 13, 2021

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Dec 9, 2020

This applies all of the rubocop fixes that can be applied via rubocop -A to the RoleManagement class. It's a mix of formatting tweaks, using alias instead of alias_method, removal of redundant self calls, plus some changes to the way some loops are handled.

Of these, I think the loop change might be the only one we might not want, in which case we should update the cop.

@djberg96
Copy link
Contributor Author

@jrafanie Look ok?

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

Sorry, for the slow review. I'm mostly good with the changes. A few comments are above.

app/models/miq_server/role_management.rb Outdated Show resolved Hide resolved
app/models/miq_server/role_management.rb Outdated Show resolved Hide resolved
app/models/miq_server/role_management.rb Outdated Show resolved Hide resolved
deactivate_roles(r.name)
end unless adds.empty?
adds = ServerRole.where(:name => (desired - current))
unless adds.empty?
Copy link
Member

Choose a reason for hiding this comment

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

Interesting...I'm curious why this was in the original code at all... .each will effectively be a no-op anyway. Not for this PR, but this line can be removed in a follow-up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted.

@Fryguy
Copy link
Member

Fryguy commented Jan 12, 2021

Of these, I think the loop change might be the only one we might not want, in which case we should update the cop.

which one is the loop change?

@djberg96
Copy link
Contributor Author

djberg96 commented Jan 12, 2021

Of these, I think the loop change might be the only one we might not want, in which case we should update the cop.

which one is the loop change?

https://github.com/ManageIQ/manageiq/pull/20881/files#diff-cbd3ec3e096cbe173ed3536046e328fd0350c56f9fcbdd90f14e255d506022c3R159-R165

@Fryguy
Copy link
Member

Fryguy commented Jan 12, 2021

That one seems fine to me...it's a straight syntactical change.

@Fryguy
Copy link
Member

Fryguy commented Jan 13, 2021

@djberg96 Can you squash commits? Otherwise the history shows a delete and a re-add which is confusing.

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM - just needs a squash.

@djberg96
Copy link
Contributor Author

@djberg96 Can you squash commits? Otherwise the history shows a delete and a re-add which is confusing.

lol, I just did that, gimme 2 minutes!

....ok done.

@miq-bot
Copy link
Member

miq-bot commented Jan 13, 2021

Checked commit https://github.com/djberg96/manageiq/commit/ef3335109175b35492b09b17dd297c3944f91ec6 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
1 file checked, 2 offenses detected

app/models/miq_server/role_management.rb

@jrafanie jrafanie merged commit 6966462 into ManageIQ:master Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants