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

[pull] master from rubocop:master #12

Open
wants to merge 1,577 commits into
base: master
Choose a base branch
from
Open

Conversation

pull[bot]
Copy link

@pull pull bot commented Feb 24, 2023

See Commits and Changes for more details.


Created by pull[bot]

Can you help keep this open source service alive? 💖 Please sponsor : )

@pull pull bot added the ⤵️ pull label Feb 24, 2023
Earlopain and others added 29 commits June 19, 2024 08:11
…essages

Changed expectations would previously output messages like this:
```
     Failure/Error: expect(options_keys.include?(:aautocorrect)).to be(true)

       expected true
            got false
```

The new expectations print the actual vs expected value:
```
     Failure/Error: expect(options_keys).to include(:aautocorrect)

       expected [:autocorrect_all, :autocorrect] to include :aautocorrect
       Diff:
       @@ -1 +1 @@
       -[:aautocorrect]
       +[:autocorrect_all, :autocorrect]
```

The same improvement also applies to something like `expect($stderr.string.include?("whatever)`.to be(true)`
or `expect(foo.nil?).to be(true)` (and probably more)
Add a spec for the changelog that cops in backticks have a department
…lter_map`

Fixes #12616

This PR makes `Style/MapCompactWithConditionalBlock` aware of `filter_map`.
…tional_block_aware_of_filter_map

[Fix #12616] Make `Style/MapCompactWithConditionalBlock` aware of `filter_map`
The only places in tests that still write to these do so to reset the output
Consistently use the console mock helper in tests
Fixes #12471.

This PR fixes false negatives for `Style/ZeroLengthPredicate`
when using safe navigation operator.
Fixes #13010.

This PR fixes an error for `Style/SuperArguments`
when the hash argument is or-assigned.
…o_length_predicate

[Fix #12471] Fix false negatives for `Style/ZeroLengthPredicate`
Fixes #13012.

This PR fixes false positives for `Style/HashExcept`
when using `reject` and calling `include?` method with bang.

As the regression tests, the incorrect existing tests are being corrected.
…h_except

[Fix #13012] Fix false positives for `Style/HashExcept`
…heses`

Fixes #13018.

This PR fixes a false positive for `Style/MethodCallWithArgsParentheses` when
`EnforcedStyle: omit_parentheses` is set and parenthesized method call is used before constant resolution.
…thod_call_with_args_parentheses_cop

[Fix #13018] Fix a false positive for `Style/MethodCallWithArgsParentheses`
#13002
SamSaffron/memory_profiler#118

This causes issues in CI since it accidentally added a dependency on base64
The next release will fix that error
Ignore latest `memory_profiler` version
…ments

[Fix #13010] Fix an error for `Style/SuperArguments`
Prevents the cause from being obscured by `uninitialized constant StrictWarnings::StringIO`
when there is a warning or syntax error in the product code.
This commit tweaks examples to return the same result for
bad examples that is incompatible with the results of good examples.
This commit clarifies and corrects the department in the list of cops.
Fixes #13023.

This PR fixes an error for `Style/SymbolProc`
when using lambda `->` with one argument and multiline `do`...`end` block.
Resolves #12557.

Currently, the server mode only auto-restarts due to incompatible versions of RuboCop core.
This PR will enable auto-restart when the SHA1 value of Gemfile.lock becomes incompatible.
If Gemfile.lock is missing, it will continue to refer to the RuboCop core version as before.

The advantage is that it can cover cases of version incompatibility caused by `bundle update`,
which will likely cover many scenarios like those in #12557.

The disadvantage is that the server mode may become slightly slower.
However, it is unlikely to be slow enough for users to notice. The trade-off is considered acceptable.

In the future, it might be possible to detect changes in rubocop configuration as well, but for now,
I'd like to see how things go with this change.
This commit suppresses the following new offenses in RuboCop RSpec 3.0.2:

```console
$ bundle exec rubocop
(snip)

spec/project_spec.rb:43:9: C: [Correctable] RSpec/PredicateMatcher: Prefer using be_nil matcher over nil?.
        expect(description.nil?).to(be(false), ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/project_spec.rb:59:9: C: [Correctable] RSpec/PredicateMatcher: Prefer using be_nil matcher over nil?.
        expect(version.nil?).to(be(false), ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/project_spec.rb:342:17: C: [Correctable] RSpec/PredicateMatcher: Prefer using include matcher over include?.
                expect(allowed_cop_names.include?(cop_name)) ...
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/project_spec.rb:344:17: C: [Correctable] RSpec/PredicateMatcher: Prefer using include matcher over include?.
                expect(entry.include?("`#{cop_name}`")) ...
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/project_spec.rb:354:17: C: [Correctable] RSpec/PredicateMatcher: Prefer using include matcher over include?.
                expect(cop_names_without_department.include?(cop_name)) ...
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/project_spec.rb:366:9: C: [Correctable] RSpec/PredicateMatcher: Prefer using include matcher over include?.
        expect(changelog.include?("[#{name}]: http")) ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1542 files inspected, 6 offenses detected, 6 offenses autocorrectable
```

https://github.com/rubocop/rubocop-rspec/releases/tag/v3.0.2
Fixes #12885.

This PR fixes a false positive for `Lint/ToEnumArguments`
when enumerator is created for another method.
…enum_arguments

[Fix #12885] Fix a false positive for `Lint/ToEnumArguments`
Follow up rubygems/rubygems#7799 (comment).

Prefer `add_dependency` over `add_runtime_dependency` as the latter is considered soft-deprecated.

```ruby
# bad
Gem::Specification.new do |spec|
  spec.add_runtime_dependency('rubocop')
end

# good
Gem::Specification.new do |spec|
  spec.add_dependency('rubocop')
end
```

rubocop/ruby-style-guide#943 has been opened for the Style Guide.
koic and others added 30 commits September 15, 2024 00:18
…ifier

[Fix #13235] Fix an error for `Style/IfUnlessModifier`
This commit fixes `Style/CollectionCompact` to ignore `delete_if`, since `delete_if` always returns self, whereas `compact!` returns `nil` if no changes are made. Therefore, the two methods are not compatible.

Co-authored-by: Koichi ITO <koic.ito@gmail.com>
Fix false positive for `Style/CollectionCompact` when using `delete_if`
This PR makes `InternalAffairs/RedundantSourceRange` aware that
`source_range` is redundant in `add_offense(node.source_range)`.
It's a heavy gem to require for something that may not even be used:

Old:
```
$ hyperfine -w 5 -r 20 "bundle exec rubocop Rakefile"
Benchmark 1: bundle exec rubocop Rakefile
  Time (mean ± σ):      3.061 s ±  0.087 s    [User: 2.494 s, System: 0.321 s]
  Range (min … max):    2.947 s …  3.275 s    20 runs
```

New:
```
$ hyperfine -w 5 -r 20 "bundle exec rubocop Rakefile"
Benchmark 1: bundle exec rubocop Rakefile
  Time (mean ± σ):      2.801 s ±  0.064 s    [User: 2.317 s, System: 0.235 s]
  Range (min … max):    2.710 s …  2.958 s    20 runs
```

~9% faster startup time

Of course, most time is still spent requiring all the various cops of rubocop itself (there are quite a few)
`require 'language-server_protocol'` on-demand
This should not be carried over to RuboCop v2
…t_source_range

Make `InternalAffairs/RedundantSourceRange` aware of `add_offense`
This matters since there are quite a few public methods per cop class.
In total, it gets called ~57k times, or ~104 times per cop class.

`method_defined?` is faster than string checking, so move that first. It also eliminates most of the checks, Base contains 98 methods.

Current:
```
$ hyperfine -w 10 -r 25 "bundle exec rubocop Rakefile --cache=false"
Benchmark 1: bundle exec rubocop Rakefile --cache=false
  Time (mean ± σ):     932.8 ms ±   7.6 ms    [User: 828.0 ms, System: 104.0 ms]
  Range (min … max):   919.5 ms … 952.2 ms    25 runs
```

With String:
```
$ hyperfine -w 10 -r 25 "bundle exec rubocop Rakefile --cache=false"
Benchmark 1: bundle exec rubocop Rakefile --cache=false
  Time (mean ± σ):     924.1 ms ±   7.8 ms    [User: 816.3 ms, System: 107.3 ms]
  Range (min … max):   912.8 ms … 941.5 ms    25 runs
```

With String + order switched:
```
$ hyperfine -w 10 -r 25 "bundle exec rubocop Rakefile --cache=false"
Benchmark 1: bundle exec rubocop Rakefile --cache=false
  Time (mean ± σ):     920.7 ms ±   6.9 ms    [User: 814.5 ms, System: 105.6 ms]
  Range (min … max):   906.9 ms … 932.5 ms    25 runs
```

~ 1.3% faster
Optimize `Base.callbacks_needed` by reordering checks
…tion-compact

Support `filter/filter!` in `Style/CollectionCompact`
Emit a deprecation for `Team.new([Cop], config)`
This has been long-marked as deprecated but not everyone has gotten the memo:
https://github.com/search?q=%2F%3C+RuboCop%3A%3ACop%3A%3ACop%2F+lang%3Aruby+-is%3Afork&type=code

Some methods _are_ deprecated but its perfectly possible to just not use any of them.
Emit a deprecation when custom cops inherit from `RuboCop::Cop::Cop`
This PR enhances the autocorrect for `Naming/InclusiveLanguage` when a sole suggestion is set.

The value of `Suggestions` is an array `['an offense']`, but since it can be determined as a sole term,
it is autocorrectable.

```yaml
Naming/InclusiveLanguage:
  FlaggedTerms:
    ' a offense':
      Suggestions:
        - an offense
```
This PR fixes the following error when using built-in LSP with Ruby 3.4dev (default by Prism).

LSP log (eglot-events-buffer):

```console
[jsonrpc] e[04:03:30.163] --> workspace/didChangeConfiguration {"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{}}}
[stderr]  [server] RuboCop 1.66.1 language server +YJIT initialized, PID 5753
(snip)

[stderr]  [server] Error NameError uninitialized constant RuboCop::LSP::Runtime::StringIO
[stderr]  [server] ["/Users/koic/src/github.com/rubocop/rubocop/lib/rubocop/lsp/runtime.rb:85:in 'RuboCop::LSP::Runtime#redirect_stdout'",
```

Ruby version:

```console
$ ruby -v
ruby 3.4.0dev (2024-09-19T18:51:21Z master 2230ac4a28) +PRISM [x86_64-darwin23]
```
…clusive_language

Enhance the autocorrect for `Naming/InclusiveLanguage`
The following `bad` cases cannot be replaced with the `good` case after v1 API:

```ruby
# bad
add_offense(node, message: message)
add_offense(node, message: message(node))

# good
add_offense(node)
```

v1 API: https://docs.rubocop.org/rubocop/v1_upgrade_notes.html

As a result, there was an incorrect detection for calls like `add_offense(message: message)` where the `message` method is the value.
As a workaround, a technique involving assignment to a local variable named `message` was used,
but fundamentally, these cases should no longer be detected by this cop.

This PR modifies the cop to detect only `add_offense(node, message: MSG)` which can still be replaced with the good cases after v1 API.
This will be slightly faster as it doesn't have to traverse the tree twice.
Benchmarks show statistical errors at best but I think the code is easier
to understand with this, so why not

The parse method never moved to keyword arguments, so no need to
do a version check for that anymore
Using an array here is costly, since it can grow rather large. Looking for
a duplicate requires a full check in the worst case, which is also the most
common one.

Instead, use a hash. Checking for existance is O(1) instead of O(n)

Current:
```
$ hyperfine -w 10 -r 25 "bundle exec rubocop Rakefile"
Benchmark 1: bundle exec rubocop Rakefile
  Time (mean ± σ):     962.2 ms ±  11.4 ms    [User: 845.0 ms, System: 116.6 ms]
  Range (min … max):   944.9 ms … 985.0 ms    25 runs
```

Without any checks:
```
$ hyperfine -w 10 -r 25 "bundle exec rubocop Rakefile"
Benchmark 1: bundle exec rubocop Rakefile
  Time (mean ± σ):     922.5 ms ±  13.8 ms    [User: 807.8 ms, System: 113.4 ms]
  Range (min … max):   899.8 ms … 951.5 ms    25 runs
```

PR:
```
$ hyperfine -w 10 -r 25 "bundle exec rubocop Rakefile"
Benchmark 1: bundle exec rubocop Rakefile
  Time (mean ± σ):     929.8 ms ±  13.1 ms    [User: 810.1 ms, System: 119.3 ms]
  Range (min … max):   912.0 ms … 959.9 ms    25 runs
```

So, about 3.5% faster for RuboCop itself.
Or in other words, ~40ms checking duplication previously, now only ~7ms.
…update

Fixes #13232.

This change was also planned in #13004. It is limited to detecting local files such as `.rubocop.yml`,
but it should reduce the need to manually restart the server mode when configuration changes are detected.
Since even in LSP, detection of configuration files beyond `.rubocop.yml` is not performed,
it is expected to cover a reasonably practical use case.

Although the logic has increased compared to #13004, no significant speed degradation is anticipated.

Since it's unclear whether tracking dependencies between configuration files is practical,
it will not be handled this, at least for now.
These two cops want to only look at a specific subset of files.
To do that, they override the `relevant_file?` method.

Instead, just treat `InternalAffairs` like a RuboCop extension and
inject the config so it goes through the usual processes.

There are more cops that could be added here (many only concern themselves
with either specs or library code) but I'll leave that for a subsequent PR.

This removes the tests from #13209, the process of config
inheritance is already well-tested and there nothing special here anymore.
This will allow the rails extension to insert the target rails version.
I ran the documentation task and confirmed there are no changes here.
Fixes #13213.

This PR fixes false positives for `Style/AccessModifierDeclarations` when `AllowModifiersOnAttrs: true`
and using splat with a percent symbol array, or with a constant.
Fixes #13202.

This PR fixes an incorrect autocorrect for `Style/CombinableLoops`
when looping over the same data with different block variable names.
…with namespaced constants

`FOO::BAR` isn't in snakecase. It should only look at the last part
Follow up #4304.

This PR tweaks the doc for `DisabledByDefault: true` in configuration.
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.