-
Notifications
You must be signed in to change notification settings - Fork 108
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
Introduce rubocop-on-rbs #608
base: main
Are you sure you want to change the base?
Conversation
- RBS/Layout/OverloadIndentation - RBS/Layout/IndentationWidth
@ksss Thanks for your contribution! Please follow the instructions below for each change. Available commandsYou can use the following commands by commenting on this PR.
|
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 |
@pocke Good idea. I updated |
@pocke |
@pocke |
bin/init_new_gem
Outdated
## | ||
# If you want to customize the style, please consult with the gem reviewers. | ||
|
||
RBS/Layout/CommentIndentation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not a good idea to list all cops in this file, because we need to update this script manually when the gem is updated.
How about writing a link to the documentation of the cops list instead of listing all cops here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was concerned about the cost of increasing the version of rubocop-on-rbs.
In rubocop, new cop is supposed to warn with Enabled: pending
. We decided to solve the problem by doing this operation in rubocop-on-rbs as well.
https://docs.rubocop.org/rubocop/versioning.html#pending-cops
Sorry for the late reply. I think this approach is good for us. But I left a code comment, please check it. |
I have corrected the points raised. |
RBS: | ||
Enabled: true | ||
RBS/Layout: | ||
Enabled: false | ||
RBS/Lint: | ||
Enabled: false | ||
RBS/Style: | ||
Enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me confirm this configuration. Is this configuration for the RBS
department necessary?
I guess we can simply remove these lines because DisabledByDefault: true
is configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the RBS
department at here.
But we will have to set Enabled: true
for the RBS
department in the .rubocop.yml under all gems.
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
andRBS/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 andRBS/Style
is not.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.