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

Specify -c .rubocop.yml for rake rubocop #6913

Closed
wants to merge 1 commit into from

Conversation

martinemde
Copy link
Member

What was the end-user or developer problem that led to this PR?

This avoids the problem where an existing .rubocop.yml is found in the user's home directory that causes errors or misconfiguration. For example, it was picking up my ~/.rubocop.yml and complaining about Rails cops.

What is your fix for the problem, implemented in this PR?

Specify the .rubycop.yml config explicitly in the rake task.

Make sure the following tasks are checked

@simi
Copy link
Member

simi commented Aug 22, 2023

Can't you just use bin/rubocop with any arguments you would like to? Maybe we can remove this rake task at all, it seems redundant and update docs.

@simi simi disabled auto-merge August 22, 2023 17:57
@deivid-rodriguez
Copy link
Member

Seems like a good idea.

@simi
Copy link
Member

simi commented Aug 22, 2023

@martinemde would you mind to update the PR? We can also merge this and I can do in followup if preferred.

@martinemde
Copy link
Member Author

I can update when I get back to the computer. Are the binstubs updated automatically somehow though?

@deivid-rodriguez
Copy link
Member

No, our current binstubs are maintained manually.

This avoids the problem where an existing .rubocop.yml is found
in the user's home directory that causes errors or misconfiguration.

It also speeds up the run by about 13%, presumably because it doesn't
need to look for the right config file.
@martinemde martinemde force-pushed the martinemde/rubocop-specify-config branch from b5ecad9 to a421ced Compare August 22, 2023 20:57
@martinemde
Copy link
Member Author

I think we should keep the rake task. rake rubocop installs the correct versions of rubocop gems according to the Gemfile at tool/bundler/lint_gems.rb, then runs rubocop using bin/rubocop. This is also part of the lint check in the github actions.

Interestingly, specifying --config reliably speeds up the run, about 13% faster overall. I added it in to the binstub instead of the rake task so that it's used every time.

@simi
Copy link
Member

simi commented Aug 22, 2023

I'm a little confused in here @martinemde. This seems problem with your specific setup. How do you handle this in other projects? Do you specify -c manually or do you change binstub to specify -c projectwise? 🤔

@martinemde
Copy link
Member Author

@simi I'm really not sure why this is showing up now. I have some basic defaults for rubocop in ~/.rubocop.yml for projects that don't have rubocop. Rubocop docs say that they ignore this file but it loads it instead. See here with --debug.

 rake rubocop\[--debug\]                                                                                                                           Tue 22 14:10:49
(in /Users/martinemde/Projects/oss/rubygems)
Bundle complete! 2 Gemfile dependencies, 14 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.
For /Users/martinemde/Projects/oss/rubygems: configuration from /Users/martinemde/Projects/oss/rubygems/.rubocop.yml
configuration from /Users/martinemde/.gem/ruby/3.2.2/gems/rubocop-performance-1.14.2/config/default.yml
configuration from /Users/martinemde/.gem/ruby/3.2.2/gems/rubocop-performance-1.14.2/config/default.yml
Default configuration from /Users/martinemde/.gem/ruby/3.2.2/gems/rubocop-1.52.1/config/default.yml
AllCops/Exclude configuration from /Users/martinemde/.rubocop.yml
Error: `Rails` cops have been extracted to the `rubocop-rails` gem.
(obsolete configuration found in /Users/martinemde/.rubocop.yml, please update it)
Finished in 0.09444499999881373 seconds
rake aborted!
Command failed with status (2): [bin/rubocop --debug...]
/Users/martinemde/Projects/oss/rubygems/Rakefile:230:in `block (2 levels) in <top (required)>'
Tasks: TOP => rubocop => rubocop:run
(See full trace by running task with --trace)

My .rubocop.yml has some Rails cops in it, so it seems that when it reads that file for a single AllCops/Exclude pattern, it's not properly ignoring unrecognized cops.

The change seems harmless to me, and it's a bit faster. It works around an apparent bug in rubocop for those of us that move between a lot of different ruby projects. If you see a negative to merging this, we can close it.

@simi
Copy link
Member

simi commented Aug 22, 2023

I'm just trying to prevent fixing user problems in rubygems codebase. Would you mind to open issue in rubocop instead? 🤔 It could be also related to strange setup of rubocop in this repo and maybe we will get some hint how to fix it. 🙏

@martinemde
Copy link
Member Author

I've tracked the problem down to an unintended behavior in rubocop. I'm fine not merging this, but I also don't think it hurts anything to merge it and it saves a small amount of time every run. I'll link the rubocop issue back here when I submit it.

@martinemde
Copy link
Member Author

martinemde commented Aug 22, 2023

This is ultimately caused by rubocop/rubocop#12147 which is, imho, a rubocop bug. Let's just leave our rake rubocop as is and close this. Thanks for pushing a bit more on this @simi.

@martinemde martinemde closed this Aug 22, 2023
@martinemde martinemde deleted the martinemde/rubocop-specify-config branch August 22, 2023 22:53
@deivid-rodriguez
Copy link
Member

@martinemde I've been there too. Thanks for fixing things at the right place.

In case it helps, see rubocop/rubocop#8176 and other linked PRs where I thought I had fixed very similar issues 😆.

@deivid-rodriguez
Copy link
Member

By the way, if this speeds up loading RuboCop, and we have an explicit binstub, I don't think it hurts at all to be explicit about the configuration.

@simi
Copy link
Member

simi commented Aug 23, 2023

This is ultimately caused by rubocop/rubocop#12147 which is, imho, a rubocop bug. Let's just leave our rake rubocop as is and close this. Thanks for pushing a bit more on this @simi.

Glad to see this fixed at proper place.

By the way, if this speeds up loading RuboCop, and we have an explicit binstub, I don't think it hurts at all to be explicit about the configuration.

Indeed, now we can open another discussion about speed performance. If that makes sense, let's merge this, just add comment in binstub explaining the reason for this.

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

3 participants