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

Fix RuboCop offences #1104

Merged
merged 64 commits into from Jun 9, 2019
Merged

Fix RuboCop offences #1104

merged 64 commits into from Jun 9, 2019

Conversation

@pirj
Copy link
Member

@pirj pirj commented Mar 8, 2019

I've kept the commits separate for easier review, with descriptive commit messages where I had to make a decision that needs explanation.

Tried to keep the changes minimal, picking the style matching the code the most (specifically with %-literals and spaces inside brackets).

RuboCop version update is better to be done simultaneously with the other rspec-* repositories since a number of rules are changed/renamed/removed, so I decided not to do that in the scope of this pull request.

@pirj pirj force-pushed the fix-rubocop-offences branch 4 times, most recently from 1498387 to f137a31 Mar 8, 2019
Copy link
Member

@JonRowe JonRowe left a comment

Went through this commit by commit, so feedback generally applies to the rules in the commit. (wow mega PR was mega!)

lib/rspec/matchers.rb Outdated Show resolved Hide resolved
.rubocop.yml Outdated Show resolved Hide resolved
.rubocop.yml Outdated Show resolved Hide resolved
.rubocop.yml Outdated Show resolved Hide resolved
spec/rspec/matchers/built_in/change_spec.rb Show resolved Hide resolved
spec/rspec/matchers/built_in/include_spec.rb Show resolved Hide resolved
.rubocop.yml Show resolved Hide resolved
spec/rspec/matchers/built_in/raise_error_spec.rb Outdated Show resolved Hide resolved
spec/rspec/matchers/dsl_spec.rb Show resolved Hide resolved
spec/rspec/matchers/dsl_spec.rb Show resolved Hide resolved
Copy link
Member Author

@pirj pirj left a comment

Will address the review shortly.
A couple of questions on how to proceed below.

.rubocop.yml Show resolved Hide resolved
.rubocop.yml Show resolved Hide resolved
.rubocop.yml Outdated Show resolved Hide resolved
.rubocop.yml Outdated Show resolved Hide resolved
.rubocop.yml Outdated Show resolved Hide resolved
spec/rspec/matchers/built_in/change_spec.rb Show resolved Hide resolved
spec/rspec/matchers/built_in/include_spec.rb Show resolved Hide resolved
spec/rspec/matchers/dsl_spec.rb Show resolved Hide resolved
spec/rspec/matchers/dsl_spec.rb Show resolved Hide resolved
spec/rspec/matchers/dsl_spec.rb Outdated Show resolved Hide resolved
@pirj pirj force-pushed the fix-rubocop-offences branch from c7c9f6c to ef35e56 Mar 10, 2019
Copy link
Member Author

@pirj pirj left a comment

Addressed the notes with no questions, squashed with the previous commits, changed commit messages where appropriate.

Please take a look at the remaining questions when you have a moment.

.rubocop.yml Show resolved Hide resolved
Copy link
Member

@benoittgt benoittgt left a comment

I am not sure various self removed are an issue if the test suite pass. This is not a blocker for me.

I would have added the link to the update/pr or specified version when disabling a Cop. For example here:

             self.a_method_not_in_the_example == "method defined in the example" # rubocop:disable Style/RedundantSelf RuboCop bug, should disappear on version update.

Like you did here

      # rubocop:disable Layout/EmptyLinesAroundArguments This is a RuboCop bug, and it's fixed in 0.65.0

But as the first point, this is not a blocker to accept this PR.

Lot's of work. 😅
Thanks for it

@pirj
Copy link
Member Author

@pirj pirj commented Mar 14, 2019

@benoittgt Thanks for the review.
Unfortunately, Style/RedundantSelf is not fixed in latest RuboCop, I temporarily updated to 0.65, it still detects where it shouldn't, and doesn't detect where it should, so I hope to re-evaluate and remove local disabling when it's fixed in RuboCop.

@pirj pirj force-pushed the fix-rubocop-offences branch from 805b5b9 to 028deeb Mar 14, 2019
@pirj
Copy link
Member Author

@pirj pirj commented Mar 14, 2019

@JonRowe Addressed a couple more remarks.

Appreciate if you check out a few of the remaining questions that I have: 1, 2, 3.

@pirj
Copy link
Member Author

@pirj pirj commented Mar 29, 2019

@JonRowe I'm up for changing the code either way, please let me know what you think.

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Apr 1, 2019

@pirj Sorry I'm very busy at the moment, I promise this is high on my list to re-review I very much appreciate your time taken here!

@pirj
Copy link
Member Author

@pirj pirj commented May 16, 2019

@JonRowe A friendly reminder. There are just three questions left (1, 2, 3), I'm up to change it the way you see fit.

My further plan is to:

  • perform fix the offences in the other RSpec repositories
  • update RuboCop version to the latest
  • extract common parts to rspec-dev's common .rubocop.yml

I'll stick to the practice I used here with the other repositories - reduce the code churn and disable the rules that would require massive codebase change.
This might take a while considering the effort to review those changes, I hope for your support in those changes.

@JonRowe
Copy link
Member

@JonRowe JonRowe commented May 17, 2019

Just a fix required to your "1" and a green build and this can be merged. Thanks so much for your patience here @pirj

pirj added 14 commits May 18, 2019
Even though the aim is to provide 1.8+ support, RuboCop of the bundled
version only supports versions 2.1-2.5.

This reduces the severity of the following offence:

rspec-expectations.gemspec:30:29: C: Gemspec/RequiredRubyVersion: required_ruby_version (1.8, declared in rspec-expectations.gemspec) and TargetRubyVersion (2.5, declared in .rubocop.yml) should be equal.
  s.required_ruby_version = '>= 1.8.7'
                            ^^^^^^^^^^
`frozen_string_literal` is not used anywhere in the code, only adding
the noise (~200 offenses) in the RuboCop check.
Specifically:
 - Style/PercentLiteralDelimiters
 - Style/UnneededPercentQ
 - Style/BarePercentLiterals

And find common ground for the usages to minimize the change.
They doesn't seem to cut it in a Gemfile with conditionals
Use default styles for Style/SpaceInsideBlockBraces
There's a really long explanation in the benchmark code. The diff size
is intolerable.
Even though this cop is coming from rspec-dev's default RuboCop
settings, in this repository it's kind of cumbersome to enforce either
of the styles the cop can enforce.
pirj added 12 commits May 18, 2019
It's quite obvious anyway from looking at the code, though it is a
balancing example and deserves to remain as is. However, instead of
excluding this file in .rubocop.yml, it makes sense to additionally
denote that the code is unreachable in the code itself.
Also, this changes reverts fdd9ef3, for which the warnings should not
appear anymore (see https://bugs.ruby-lang.org/issues/10661).
Aruba creates some tmp files that don't pass the checks. Also, git
ignored bin folder (of an unknown origin, probably part of how rspec-dev
checks out and initializes repositories) contains a number of offences.

    bin/bundle:34:25: C: Style/PerlBackrefs: Avoid the use of Perl-style backrefs.
                            ^^
    bin/bundle:89:187: C: Metrics/LineLength: Line is too long. [190/186]

    tmp/aruba/rspec_expectations_spec.rb:48:4: C: Layout/TrailingBlankLines: Final newline missing.

It doesn't make sense to fix them, since those files are not meant to be
committable.
@pirj pirj force-pushed the fix-rubocop-offences branch from 2b4e1ed to c304482 May 18, 2019
pirj added a commit to pirj/rspec-expectations that referenced this issue May 21, 2019
The test was a bit confusing.
The point of it is that if a `matches?` is defined, and `===` is not
defined, it should not be used as an argument matcher.

rspec#1104 (comment)
pirj added 5 commits May 21, 2019
Previously CI check was only checking `lib` directory
It is a bug in RuboCop 0.52.1, fixed in 0.65.0
Adding a local disabling, since newer RuboCop will report an
unnecessary disabling, and it could be spotted and removed.
The test was a bit confusing.
The point of it is that if a `matches?` is defined, and `===` is not
defined, it should not be used as an argument matcher.

rspec#1104 (comment)
@pirj pirj force-pushed the fix-rubocop-offences branch from eb1e325 to 8d877a8 May 21, 2019
@pirj
Copy link
Member Author

@pirj pirj commented May 28, 2019

@JonRowe Addressed the remaining issue. Please take another look.

@benoittgt
Copy link
Member

@benoittgt benoittgt commented May 29, 2019

I reviewed last commits. LGTM. Thanks for the hard work @pirj

@pirj
Copy link
Member Author

@pirj pirj commented Jun 3, 2019

@JonRowe Please take another look.

JonRowe
JonRowe approved these changes Jun 9, 2019
@JonRowe JonRowe merged commit 23c11fc into rspec:master Jun 9, 2019
2 checks passed
@JonRowe
Copy link
Member

@JonRowe JonRowe commented Jun 9, 2019

Thanks @pirj !

@pirj pirj deleted the fix-rubocop-offences branch Jun 9, 2019
@pirj
Copy link
Member Author

@pirj pirj commented Jun 9, 2019

Thanks for accepting @JonRowe !

yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this issue Oct 6, 2021
* Specify target Ruby version for RuboCop

Even though the aim is to provide 1.8+ support, RuboCop of the bundled
version only supports versions 2.1-2.5.

This reduces the severity of the following offence:

rspec-expectations.gemspec:30:29: C: Gemspec/RequiredRubyVersion: required_ruby_version (1.8, declared in rspec-expectations.gemspec) and TargetRubyVersion (2.5, declared in .rubocop.yml) should be equal.
  s.required_ruby_version = '>= 1.8.7'
                            ^^^^^^^^^^

* Fix Style/Encoding offence

* Set FrozenStringLiteralComment style to never

`frozen_string_literal` is not used anywhere in the code, only adding
the noise (~200 offenses) in the RuboCop check.

* Fix Layout/EmptyLines offence

* Fix Gemspec/OrderedDependencies offence

* Fix percent literal related offences

Specifically:
 - Style/PercentLiteralDelimiters
 - Style/UnneededPercentQ
 - Style/BarePercentLiterals

And find common ground for the usages to minimize the change.

* Disable Gemfile cops

They doesn't seem to cut it in a Gemfile with conditionals

* Fix Layout/SpaceAfterComma offences

* Disable Lint/AmbiguousRegexpLiteral cop for step definitions

* Fix Style/SpaceInsideBlockBraces offences

Use default styles for Style/SpaceInsideBlockBraces

* Fix Layout/TrailingBlankLines offences

* Disable Style/BlockComments cop

There's a really long explanation in the benchmark code. The diff size
is intolerable.

* Exclude Gemfile from Security/Eval check

* Disable Layout/AlignParameters locally

Even though this cop is coming from rspec-dev's default RuboCop
settings, in this repository it's kind of cumbersome to enforce either
of the styles the cop can enforce.

* Fix Style/ColonMethodCall offences

* Exclude specs from Metrics/ModuleLength check

* Fix Layout/SpaceInsideHashLiteralBraces offences

Using the default styles reduces the total change of the code required,
since with defaults the number of offences is the lowest.

                     | `space` (default) | `no_space` | `compact`
 ------------------- | ----------------- | ---------- | ---------
`no_space` (default) | 92                | 126        | 92
             `space` | 105               | 139        | 105

* Exclude specs from Metrics/BlockLength check

* Disable Style/WordArray cop

* Fix Layout/SpaceBeforeBlockBraces offences

* Fix Style/AsciiComments offence

Looks even more furious now.

* Disable Lint/HandleExceptions for benchmarks

* Disable Lint/HandleExceptions in the code

This handling is there to avoid odd messages when this example fails

* Disable Style/Semicolon

Sometimes it's more indicative.

It is very tempting to simplify

    expect {
      x = 0
      expect { x += 1; exit }.to change { x }.and cause_call_stack_jump
    }.to raise_error(/no match results, [\.\w\s]+ declare `expects_call_stack_jump\?`/)

down to

    expect {
      expect { exit }.to change { }.and cause_call_stack_jump
    }.to raise_error(/no match results, [\.\w\s]+ declare `expects_call_stack_jump\?`/)

however, maybe I just don't see how `x` facilitates the understanding
how a call stack jump is detected in a composable matcher.

* Exclude specs and benchmarks from Style/SingleLineMethods check

* Fix Style/NumericLiterals offences

* Exclude specs and benchmarks from Style/RescueModifier check

* Fix Performance/StringReplacement offence

* Fix Style/LineEndConcatenation offences

* Auto-correct Lint/UnusedBlockArgument offences

I'm not particularly sure about this commit. Probably some of the
examples are there specifically to indicate a bad code and how RSpec is
able to handle it anyway.

* Fix Style/RedundantSelf offences

* Fix Style/EvenOdd offence

* Exclude specs from Style/MultilineBlockChain check

* Fix some Lint/IneffectiveAccessModifier offences

* Fix Style/MixinGrouping offence

* Exclude specs from Style/ClassAndModuleChildren check

I tried fixing this across all specs, since it also triggers
Metrics/ModuleLength cop, but it turns out that this style is
specifically useful if a number of constants are used in the tests,
e.g. it's possible to specify
`RSpec::Expectations::MultipleExpectationsNotMetError` just by the class
name, leaving out the namespace.

* Exclude specs from Lint/AmbiguousBlockAssociation check

The following notation is quite widespread

    expect { string << "c" }.to change { string }

and it would look extremely odd being "fixed":

    expect { string << "c" }.to(change { string })

Even though sometimes it's hard to figure out what is the receiver of
the message with composable matchers, the matchers are built the way it
doesn't really matter.

* Exclude specs from Style/BracesAroundHashParameters check

This style makes total sense in specs, e.g.:

    expect({:a => 1, :b => 2}).to include(:a => 1, :b => 2)

indicates that `actual` is a hash.

* Exclude specs from Layout/SpaceInsideParens check

This style looks quite nicely:

    by( a_value_within(0.1).of(0.5) )

especially when chained with another matcher down the lines.

An example with `compose` in spec/rspec/matchers/built_in/compound_spec.rb:56:16

    expect {
      w += 1; x += 2; y += 3; z += 4
    }.to(                 combine(
      change { w }.to(1), combine(
      change { x }.to(2), combine(
      change { y }.to(3),
      change { z }.to(4) ) ) )
    )

is there for a single reason - lack of varargs in earlier, but still
supported Ruby versions.

* Exclude specific file from Style/EvalWithLocation check

The `__LINE__` would be completely useless for evals in this spec.

* Fix Layout/SpaceInsideArrayLiteralBrackets offences

One of the "offences" that went into the exclude file is there to
visually demonstrate what is in the `actual`.

* Disable Style/GlobalVars inline

I tried to keep off of in-code disabling of the cops, but since this
specific line is a hack, probably it makes sense to highlight it as
such.

* Disable Gemspec/RequiredRubyVersion

Minimum Targeted Ruby version of used RuboCop is 2.1, while even it was
dropped in rubocop/rubocop@a707c23
2.2 is past the support phase, and so will be 2.3 by the end of March
2019.

* Fix Layout/SpaceInsideReferenceBrackets offence

* Fix Style/Attr offence

* Locally disable Style/RedundantException offences

As per https://github.com/rspec/rspec-expectations/pull/1104/files#r263923497
discussion it's better to be explicit, and to suppress the cop

* Fix Layout/EmptyLinesAroundModuleBody offences

* Fix Style/NumericLiteralPrefix offences

* Fix Layout/SpaceAroundEqualsInParameterDefault offences

* Fix Layout/EmptyLinesAroundClassBody offence

* Fix Style/EmptyLiteral offences

* Fix Lint/UnusedMethodArgument offences

* Fix Performance/RedundantBlockCall offence

* Explicitly denote that the code is unreachable

It's quite obvious anyway from looking at the code, though it is a
balancing example and deserves to remain as is. However, instead of
excluding this file in .rubocop.yml, it makes sense to additionally
denote that the code is unreachable in the code itself.

* Locally disable Lint/RescueException offence

Discussion: https://github.com/rspec/rspec-expectations/pull/1104/files#r263990646

* Fix Style/MethodCallWithoutArgsParentheses offences

Also, this changes reverts fdd9ef3862, for which the warnings should not
appear anymore (see https://bugs.ruby-lang.org/issues/10661).

* Fix Layout/IndentationConsistency offence

* Exclude ignored paths from RuboCop checks

Aruba creates some tmp files that don't pass the checks. Also, git
ignored bin folder (of an unknown origin, probably part of how rspec-dev
checks out and initializes repositories) contains a number of offences.

    bin/bundle:34:25: C: Style/PerlBackrefs: Avoid the use of Perl-style backrefs.
                            ^^
    bin/bundle:89:187: C: Metrics/LineLength: Line is too long. [190/186]

    tmp/aruba/rspec_expectations_spec.rb:48:4: C: Layout/TrailingBlankLines: Final newline missing.

It doesn't make sense to fix them, since those files are not meant to be
committable.

* Add RuboCop check to default rake task

* Fix typos and RuboCop reference in docs

* Add a check for RuboCop binstub

* Tell RuboCop to check all files

Previously CI check was only checking `lib` directory

* Locally disable Layout/EmptyLinesAroundArguments

It is a bug in RuboCop 0.52.1, fixed in 0.65.0
Adding a local disabling, since newer RuboCop will report an
unnecessary disabling, and it could be spotted and removed.

* Simplify regression test

The test was a bit confusing.
The point of it is that if a `matches?` is defined, and `===` is not
defined, it should not be used as an argument matcher.

rspec/rspec-expectations#1104 (comment)

---
This commit was imported from rspec/rspec-expectations@23c11fc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants