gem_rbs_collection icon indicating copy to clipboard operation
gem_rbs_collection copied to clipboard

Introduce rubocop-on-rbs

Open ksss opened this issue 1 year ago • 7 comments

Introduce rubocop-on-rbs Implements of #601

As an introduction, we automated the pointing out by rubocop-on-rbs to activesupport.

.rubocop.yml

Root

I have included common settings in the project root .rubocop.yml, but disabled all Cop.

Each gems

I have enabled RBS/Layout and RBS/Lint only in activesupport. With other gems, rubocop detection does not work depending on the route configuration. This means that the owner of the gem can optionally set .rubocop.yml to enable the check.

CI

By running RuboCop in CI, automatic feedback can be displayed on PRs. This frees reviewers from pointing out trivial issues and allows them to focus on essential reviews.

Operation check

You can see that RBS/Layout is enabled and RBS/Style is not.

image

When Updating rubocop-on-rbs

When New Cops Are Added via gem Update

New Cops are disabled by default. There is no burden from the gem update. If you want to enable new Cops, you need to do so for each gem.

When the Behavior of Existing Cops Changes

You need to make sure that RuboCop passes for all gems (this is assumed to be a rare case).

When Adding a New gem

Use init_new_gem to add a .rubocop.yml file. If unnecessary, simply delete this file. This file enables most Cops by default.

ksss avatar Jun 17 '24 13:06 ksss

@ksss Thanks for your contribution!

Please follow the instructions below for each change. See also: https://github.com/ruby/gem_rbs_collection/blob/main/docs/CONTRIBUTING.md

Available commands

You can use the following commands by commenting on this PR.

  • /merge: Merge this PR if CI passes

activesupport

You changed RBS files for an existing gem. You can merge this PR yourself because you are a reviewer of this gem. Just comment /merge to merge this PR.

You can also request a review from other reviewers if you want.


You changed non-gem files.

@pocke, please review and approve the changes.

github-actions[bot] avatar Jun 17 '24 14:06 github-actions[bot]

Thanks for your pull request.

Decreasing development speed is my main concern when introducing RuboCop. But probably this opt-in approach can reduce the negative impact because RuboCop is enabled only for maintainer who want to use it.

By the way, we can probably update the init_new_gem script to create a boilerplate of .rubocop.yml.

pocke avatar Jun 24 '24 06:06 pocke

@pocke Good idea. I updated init_new_gem. And to facilitate the update of rubocop-on-rbs, I have rewritten the settings on a per-cop basis.

ksss avatar Jun 24 '24 14:06 ksss

@pocke I also thought about the operation of updating rubocop-on-rbs. Do you have any concerns?

ksss avatar Jun 28 '24 01:06 ksss

@pocke Are you positive or negative about this change? I would like to hear your opinion.

ksss avatar Sep 30 '24 07:09 ksss

Sorry for the late reply. I think this approach is good for us. But I left a code comment, please check it.

pocke avatar Oct 15 '24 04:10 pocke

I have corrected the points raised. Also, changes to activesupport were rescinded to simplify the PR.

ksss avatar Oct 20 '24 13:10 ksss

Thanks for your review, @pocke!

This PR still needs approval from the administrators. The admin should review this PR and merge it manually. Please get approval from the reviewers.

github-actions[bot] avatar Dec 24 '24 07:12 github-actions[bot]