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

Do not fork off extra processes if only a single file needs inspection #10889

Closed
wants to merge 37 commits into from

Conversation

WJWH
Copy link
Contributor

@WJWH WJWH commented Aug 8, 2022

This MR makes it so that if there is only a single file to be inspected, we do not split off extra worker processes to do the work. This is because doing the work in a separate process can never be faster than doing it inline. I benchmarked this change on my dev machine (with 6 physical cores) and it saves 40-50 ms per run (only if only a single file is inspected of course, otherwise it will take as long as before).

I did not know what to update the test with, so I just made it expect the new text. Let me know if this should be different.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Updated tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

WJWH and others added 30 commits August 8, 2022 18:56
…umentIndentation`

Fixes rubocop#10830.

This PR fixes an incorrect autocorrect for `Layout/FirstArgumentIndentation` when specifying
`EnforcedStyle: with_fixed_indentation` of `Layout/ArgumentAlignment` and
`EnforcedStyle: consistent` of `Layout/FirstArgumentIndentation` and
enabling `Layout/FirstMethodArgumentLineBreak`.
Fixes rubocop#10843.

This PR fix a false positive for `Style/HashExcept` when using
`reject` and calling `include?` method with symbol array and
second block value.
…line option

Fix rubocop#10771.

This PR makes server mode aware of `--cache-root` command line option.

The logic for getting `--cache-root` from command line option is original to make
the implementation lighter. Because the client of server mode should be lightweight.
rubocop#10842 needs a different solution than `--cache-root` option.
I'm working on it separately from solving this issue.
Previously the wording implied that between major versions the API won't change.
But it's pretty clear from the surrounding wording and general semvar concept
that the API _will_ change between major versions... it's only between minor/patch
versions that it won't change.
This PR is fix an autocorrect for `Style/RedundantSort` with predicate operator

If you have a code like the following:
```ruby
[1, 2, 3]
  .sort_by { |x| x.length }
  .first || []
```

After autocorrection it will look like this:
```ruby
[1, 2, 3]
  .sort_by { |x| x.length }
  || []
```

This is a syntax error.
```
Error: : eval:3: syntax error, unexpected '|', expecting end-of-input
  || []
  ^
 (SyntaxError)
 ```
…d two env vars (rubocop#10852)

Fixes rubocop#10842 and follow up rubocop#10849.

This PR makes server mode aware of `CacheRootDirectory` config option value,
`RUBOCOP_CACHE_ROOT`, and `XDG_CACHE_HOME` environment variables.

`RuboCop::ConfigLoader` and `RuboCop::ResultCache` classes to get these values has many dependencies.
So, it is not suitable for use in server mode where speed is required.

This solution is to extract `ConfigFinder` class from `ConfigLoader` class and `CacheConfig` class
from `ResultCache` class.
This allows server mode to depend on lightweight `ConfigFinder` and `CacheConfig` classes.
Shared libraries often contain timestamps of when they were compiled and other non-stable data
hence would often produce different hashes on different machines
Fixes rubocop#10864.

This PR fixes a false positive for `Style/SymbolProc` when using `Hash#reject`.
The same is true for `Haash#select`.

```ruby
{foo: :bar}.reject { p _1 } # `_1` is `:foo`
{foo: :bar}.select { p _1 } # `_1` is `:foo`
{foo: :bar}.detect { p _1 } # `_1` is `[:foo, :bar]`
{foo: :bar}.map { p _1 }    # `_1` is `[:foo, :bar]`
```

It fixes `on_block` method because the same issue occurs in normal blocks.

```ruby
{foo: :bar}.select {|item| item.nil? } #=> no erros.
{foo: :bar}.select(&:nil?)             #=> wrong number of arguments (given 1, expected 0) (ArgumentError)
```

`Style/SymbolProc` is already marked as unsafe, but it avoids avoidable issues.
…hen there is a hash or an array at return location of method
…eprecated

Fixes rubocop#10871 and utkarsh2102/rubocop-packaging#44.

This PR restores `RuboCop::ConfigLoader.project_root` as deprecated.

It fixes the following build error.

```console
% cd path/to/github.com/utkarsh2102/rubocop-packaging
% bundle update && bundle exec rake
(snip)

Failures:

  1) RuboCop::Cop::Packaging::BundlerSetupInTests when `require
  bundler/setup` is used in a Rakefile does not register an offense
     Failure/Error: let(:project_root) {
  RuboCop::ConfigLoader.project_root }

     NoMethodError:
       undefined method `project_root' for RuboCop::ConfigLoader:Class
     # ./spec/rubocop/cop/packaging/bundler_setup_in_tests_spec.rb:6:in
       `block (2 levels) in <top (required)>'
     # ./spec/rubocop/cop/packaging/bundler_setup_in_tests_spec.rb:39:in
       `block (3 levels) in <top (required)>'
     # ./spec/rubocop/cop/packaging/bundler_setup_in_tests_spec.rb:43:in
       `block (3 levels) in <top (required)>'
```

I considered a soft deprecation, but decided to issue a warning as it
is probably of limited use.
…ating `.rubocop_todo.yml`

Create two files with the following code:
```ruby
def fooBar; end
```

Set the following in .rubocop.yml:
```yml
Style/MethodName:
  IgnoredPatterns:
    - pattern1
    - pattern2
```

Run `rubocop --auto-gen-config`
Then duplicate configuration values are enumerated in `.rubocop_todo.yml` as follows:
```yml
# Offense count: 2
# Configuration parameters: EnforcedStyle, AllowedPatterns, IgnoredPatterns.
# SupportedStyles: snake_case, camelCase
# AllowedPatterns: pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2, pattern1, pattern2
Naming/MethodName:
  Exclude:
    - 'test1.rb'
    - 'test2.rb'
```
dvandersluis and others added 7 commits August 8, 2022 21:54
… over `Style/RedundantParentheses`.

`Style/TernaryParentheses` allows RuboCop to require ternary conditions are wrapped in parentheses, but this behaviour doesn't take effect when `Style/RedundantParentheses` is also enabled, in order to prevent an infinite loop.

However, it makes more sense for `TernaryParentheses` to take priority here and have `RedudantParentheses` allow the parentheses instead of creating an infinite autocorrection loop.

This change moves the infinite loop protection into `RedundantParentheses` instead.
…lVariable` conditional statement and block variable
- Resolves what seems to be an unintentional typo where `elif` (as a method call) is being use in place of the `elsif` keyword.
Follow up rubocop/rubocop-ast#230 and fixes rubocop#10552.

This commit fixes a false positive for `Lint/OutOfRangeRegexpRef` when using
fixed-encoding regopt.
@WJWH
Copy link
Contributor Author

WJWH commented Aug 8, 2022

Sigh, only commit 39a942d contains the changes I made. The rest comes from rebasing where I think I did something wrong when adding this repo as a remote 🤔

EDIT: Also the one failing test in the CI doesn't seem to be related to my changes.

@dvandersluis
Copy link
Member

dvandersluis commented Aug 8, 2022

I like this idea but you have to clean up the commits. It looks like you somehow got a merge of master in between your two commits a653a0e and 39a942d. You should be able to do a git rebase --interactive against master and remove the other commits, but if you can’t figure that out you could create a new branch off of master and cherry pick those two commits (and then squash them together)

git checkout master
git pull master

# interactive rebase (it’ll open an editor with instructions)
git rebase master --interactive

# or, create a new branch and cherry pick
git checkout -b [branch name]
git cherry pick a653a0e
git cherry pick 39a942d

NB: if you create a new branch you'll have to close this PR and open a new one.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 9, 2022

The proposed change looks good to me. Don't forget to add a changelog entry about it.

@WJWH WJWH mentioned this pull request Aug 10, 2022
8 tasks
@WJWH
Copy link
Contributor Author

WJWH commented Aug 10, 2022

I made #10903 to replace this PR, it contains only the relevant changes.

@WJWH WJWH closed this Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet