Enable remaining disabled Rubocop rules and fix violations #2549
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Great work @floehopper |
> Lint/AmbiguousOperator: Ambiguous negative number operator. > Parenthesize the method arguments if it's surely a negative number operator, > or add a whitespace to the right of the - if it should be a subtraction. > (https://github.com/bbatsov/ruby-style-guide#parens-as-args)
> Lint/AmbiguousRegexpLiteral: Ambiguous regexp literal. > Parenthesize the method arguments if it's surely a regexp literal, > or add a whitespace to the right of the / if it should be a division.
> Lint/AssignmentInCondition: Assignment in condition > - you probably meant to use ==. > (https://github.com/bbatsov/ruby-style-guide#safe-assignment-in-condition) > Don't use the return value of = (an assignment) in conditional expressions > unless the assignment is wrapped in parentheses. This is a fairly popular > idiom among Rubyists that's sometimes referred to as safe assignment in > condition. # bad (+ a warning) if v = array.grep(/foo/) do_something(v) # some code end # good (MRI would still complain, but RuboCop won't) if (v = array.grep(/foo/)) do_something(v) # some code end # good v = array.grep(/foo/) if v do_something(v) # some code end Note that I've also updated the regression test checksums for `calculate-your-holiday-entitlement`, because this commit doesn't cause any change in behaviour.
> Lint/HandleExceptions: Do not suppress exceptions. > (https://github.com/bbatsov/ruby-style-guide#dont-hide-exceptions) # bad begin # an exception occurs here rescue SomeError # the rescue clause does absolutely nothing end Note that I've fixed these somewhat arbitrarily by printing the exception inside the `rescue` block: * As far as I know the `links:check` rake task is not currently used and in any case printing to standard out should be OK. * `ActionDispatch::IntegrationTest#wait_until` is used via `#assert_current_url` but since all invocations are within tests, it seems legitimate to print the exception to standard out.
> Lint/Loop: Use Kernel#loop with break rather than begin/end/until(or while). > (https://github.com/bbatsov/ruby-style-guide#loop-with-break) # bad begin puts val val += 1 end while val < 0 # good loop do puts val val += 1 break unless val < 0 end
> Lint/ParenthesesAsGroupedExpression: (...) interpreted as grouped expression. > (https://github.com/bbatsov/ruby-style-guide#parens-no-spaces) > Do not put a space between a method name and the opening parenthesis. # bad f (3 + 2) + 1 # good f(3 + 2) + 1 Note that I've also updated the regression test checksums for `calculate-agricultural-holiday-entitlement`, because this commit doesn't cause any change in behaviour.
> Lint/RescueException: Avoid rescuing the Exception class. > Perhaps you meant to rescue StandardError? > (https://github.com/bbatsov/ruby-style-guide#no-blind-rescues) > Avoid rescuing the Exception class. This will trap signals and calls to exit, > requiring you to kill -9 the process. # bad begin # calls to exit and kill signals will be caught (except kill -9) exit rescue Exception puts "you didn't really want to exit, right?" # exception handling end # good begin # a blind rescue rescues from StandardError, not Exception as many # programmers assume. rescue => e # exception handling end # also good begin # an exception occurs here rescue StandardError => e # exception handling end
> Lint/ShadowingOuterLocalVariable: Shadowing outer local variable > - responses_and_expected_results.
> Lint/UselessAssignment: Useless assignment to variable > (https://github.com/bbatsov/ruby-style-guide#underscore-unused-vars) Note that I've also updated the regression test checksums for the following flows, because this commit doesn't cause any change in behaviour. * `help-if-you-are-arrested-abroad` * `legalisation-document-checker` * `simplified-expenses-checker`
This test looks as if it's a duplicate of another one:
should "calculate pay due on the last day of the month"
So I decided to remove it rather than come up with a set of assertions.
This was highlighted by the enabling of the Lint/UselessAssignment cop.
This test was incorrectly trying to add an inappropriate response, `no_embassy_result`, and it had no assertion. The test was passing previously, because `add_response` doesn't cause any processing of the responses. It looks to me as if the call to `add_response` was intended to be a call to `assert_current_node`. This was highlighted by the enabling of the Lint/UselessAssignment cop.
> Style/ClassVars: Replace class var @@historical_minimum_wage_data with a class > instance var. (https://github.com/bbatsov/ruby-style-guide#no-class-vars) I've converted the instance method into a class method and the class variable into a class instance variable. Note that I've also updated the regression test checksums for the following flows, because this commit doesn't cause any change in behaviour. * `am-i-getting-minimum-wage` * `minimum-wage-calculator-employers`
> Style/MethodCalledOnDoEndBlock: Avoid chaining a method call on a do...end > block. (https://github.com/bbatsov/ruby-style-guide#single-line-blocks) Note that I've also updated the regression test checksums for the following flows, because this commit doesn't cause any change in behaviour. * `am-i-getting-minimum-wage` * `minimum-wage-calculator-employers` * `maternity-paternity-calculator`
> Style/MultilineTernaryOperator: Avoid multi-line ?: (the ternary operator); > use if/unless instead. > (https://github.com/bbatsov/ruby-style-guide#no-multiline-ternary) > Avoid multi-line ?: (the ternary operator); use if/unless instead. Note that I've also updated the regression test checksums for `simplified-expenses-checker`, because this commit doesn't cause any change in behaviour.
> Style/StructInheritance: Don't extend an instance initialized by Struct.new. > (https://github.com/bbatsov/ruby-style-guide#no-extend-struct-new) > Don't extend an instance initialized by Struct.new. Extending it introduces a > superfluous class level and may also introduce weird errors if the file is > required multiple times. # bad class Person < Struct.new(:first_name, :last_name) end # good Person = Struct.new(:first_name, :last_name)
These should have been removed when the flow was removed in this commit [1]. This came to light when enabling the `Style/StructInheritance` cop and fixing the violation. [1]: 2b1b727
All the default cops are now enabled and so there's no longer any need for this extra configuration. All this was originally added in this commit [1]. [1]: 5e48f3a
|
Rebasing against master and force-pushing in preparation for merging. |
floehopper
force-pushed
the
enable-remaining-disabled-rubocop-rules-and-fix-violations
branch
from
232367b to
30c6a48
Compare
floehopper
deleted the
enable-remaining-disabled-rubocop-rules-and-fix-violations
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Trello card: https://trello.com/c/IgN5oYCy
In #2418 an auto-generated Rubocop configuration (
.rubocop_todo.yml) was generated to disable cops for files where violations were occurring in the existing code. We have already enabled some of these cops in previous pull requests, notably #2474. This PR enables all the remaining disabled cops and fixes violations:In fixing the violations, a few other small issues were uncovered and I decided it was better to address them here, rather than create several other very small pull requests.
Finally I have removed the
.rubocop_todo.ymlfile and its associated documentation.External changes
There should be no change in external behaviour. This is just an internal refactoring.