Update Rubocop and fix offenses #735
Conversation
murny
commented
May 28, 2018
•
murny
commented
- Upgrade Rubocop from 0.51 to 0.56
| - app/models/concerns/item_properties.rb | ||
|
|
||
| # TODO: Review and fix these | ||
| Naming/UncommunicativeMethodParamName: |
There was a problem hiding this comment.
Rubocop doesn't like parameters with less then 3 letters (fq, qf, q, ip, as, etc)
Naming/UncommunicativeMethodParamName: Method parameter must be at least 3 characters long.
Disabled the offenders for now
| Rails/FindEach: | ||
| Enabled: false | ||
|
|
||
| # TODO: False postivies on headers, remove this once on rubocop > 0.56.0 as fixed in next release |
There was a problem hiding this comment.
This got fixed upstream: rubocop/rubocop#5888
Disabling offender for now, can remove this once we on 0.56+
| @@ -74,15 +74,45 @@ Naming/FileName: | |||
| - Rakefile | |||
| - Gemfile | |||
|
|
|||
| # TODO: Review and fix these | |||
| Naming/MemoizedInstanceVariableName: | |||
There was a problem hiding this comment.
Rubocop wants memorized instance variable to be the same name as the method name.
For example
def count
@count ||= super
end
instead of
def count
@count_cache ||= super
end
Naming/MemoizedInstanceVariableName: Memoized variable @count_cache does not match method name count. Use @count instead. @count_cache ||= super
We have a few offenders of this. Disabling for now
| # There comes a point where I question Rubocop's maintainer's sanity | ||
| Rails/UnknownEnv: | ||
| Enabled: false | ||
|
|
||
| # Autocorrecting this actively breaks code | ||
| Rails/FindEach: | ||
| Style/AsciiComments: |
There was a problem hiding this comment.
Most of these diffs below are for reordering these in alphabetical order
| @@ -38,7 +38,7 @@ class Admin::UsersControllerTest < ActionDispatch::IntegrationTest | |||
| assert_equal I18n.t('admin.users.show.unsuspend_flash'), flash[:notice] | |||
|
|
|||
| user.reload | |||
| refute user.suspended? | |||
| assert_not user.suspended? | |||
There was a problem hiding this comment.
rubocop prefers assert_not instead of the refute style assertions
| @@ -266,7 +267,7 @@ def license_for_uri(uri) | |||
| code = CONTROLLED_VOCABULARIES[:license].from_uri(uri) | |||
| license = URI_CODE_TO_LICENSE[code].to_s | |||
|
|
|||
| license.blank? ? 'unselected' : license | |||
| license.presence || 'unselected' | |||
There was a problem hiding this comment.
.presence will return itself if true, otherwise nil. Rubocop autocorrects these
| @@ -159,9 +159,9 @@ def reify_result_set | |||
|
|
|||
| def sort_clause | |||
| model = criteria[:restrict_to_model].first.owning_class | |||
| indexes = criteria[:sort].present? ? criteria[:sort] : model.default_sort_indexes | |||
| indexes = criteria[:sort].presence || model.default_sort_indexes | |||
There was a problem hiding this comment.
Am I the only one that finds presence less readable?
I'm not married to removing it but I'm not loving it either
There was a problem hiding this comment.
I'm indifferent? So if anyone feels strongly about this, we can disable
There was a problem hiding this comment.
Sorry, no strong opinion.
In honesty, I had to look presence up -- would be nice if the method had a more illustrative name (oh well).
There was a problem hiding this comment.
99% of my feeling on this is "I wish it had a slightly different name".
I guess we live with it. My eyes aren't bleeding or anything.
There was a problem hiding this comment.
In this very particular case, maybe fetch would be more readable?
There was a problem hiding this comment.
Between the two, I guess I like present more. It's kind of a "which do I like finding more, a used bandaid or a toe-nail" situation though
bin/yarn
Outdated
| exit 1 | ||
| end | ||
| exec 'yarnpkg', *ARGV | ||
| rescue Errno::ENOENT |
There was a problem hiding this comment.
I think rubocop screwed this up. Now if APP_ROOT doesn't exist (possible if you're cd'd into the dir in one terminal after rm-ing it in another), that error will get rescued here and the wrong error message displayed.
It's a real corner-case, admittedly.
There was a problem hiding this comment.
Can revert, rubocop autocorrected this. So can disable this file for whatever it's doing.
