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

Fixes #19887 - Disable Style/ParenthesesAroundCondition cop #5706

Merged
merged 1 commit into from
Jul 1, 2018
Merged

Fixes #19887 - Disable Style/ParenthesesAroundCondition cop #5706

merged 1 commit into from
Jul 1, 2018

Conversation

mmoll
Copy link
Contributor

@mmoll mmoll commented Jun 15, 2018

No description provided.

@theforeman-bot
Copy link
Member

Issues: #19887

@mmoll
Copy link
Contributor Author

mmoll commented Jun 15, 2018

See https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/ParenthesesAroundCondition - I think parentheses increase understandability.

👍 for disabling - and thus merging.
👎 for enabling

@@ -11,23 +11,23 @@ def setup
get :index
assert_response :success
fact_values = ActiveSupport::JSON.decode(@response.body)['results']
expected_hash = {@host.name=>{"kernelversion"=>"2.6.9"}}
expected_hash = {@host.name => {"kernelversion" => "2.6.9"}}
Copy link
Member

Choose a reason for hiding this comment

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

are changes in this file related? (I like them :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ares that's the first of the two commits.

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Looks like most people prefer disabling it, and even if not, there's no consensus on enabling so I guess disable it is :)

@tbrisker tbrisker merged commit 1b94e8a into theforeman:develop Jul 1, 2018
@mmoll mmoll deleted the moar_rubocop6 branch July 1, 2018 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants