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

Introduce rubocop-on-rbs #608

Merged
merged 15 commits into from
Dec 24, 2024
Merged

Introduce rubocop-on-rbs #608

merged 15 commits into from
Dec 24, 2024

Conversation

ksss
Copy link
Collaborator

@ksss ksss commented Jun 17, 2024

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 added 4 commits June 17, 2024 22:25
@ksss ksss force-pushed the rubocop-on-rbs branch from 7adb1cf to 94b1e2c Compare June 17, 2024 13:25
@ksss ksss force-pushed the rubocop-on-rbs branch from 8cd94b6 to 3675b01 Compare June 17, 2024 13:35
@ksss ksss marked this pull request as ready for review June 17, 2024 14:01
@ksss ksss requested a review from pocke as a code owner June 17, 2024 14:01
Copy link

@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.

@pocke
Copy link
Member

pocke commented Jun 24, 2024

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.

@ksss
Copy link
Collaborator Author

ksss commented Jun 24, 2024

@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
Copy link
Collaborator Author

ksss commented Jun 28, 2024

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

@ksss
Copy link
Collaborator Author

ksss commented Sep 30, 2024

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

bin/init_new_gem Outdated
##
# If you want to customize the style, please consult with the gem reviewers.

RBS/Layout/CommentIndentation:
Copy link
Member

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?

Copy link
Collaborator Author

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

@pocke
Copy link
Member

pocke commented Oct 15, 2024

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

@ksss
Copy link
Collaborator Author

ksss commented Oct 20, 2024

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

Comment on lines +23 to +30
RBS:
Enabled: true
RBS/Layout:
Enabled: false
RBS/Lint:
Enabled: false
RBS/Style:
Enabled: false
Copy link
Member

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.

Copy link
Collaborator Author

@ksss ksss Oct 29, 2024

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the problem. When DisabledByDefault is true, RuboCop does not enable cops under RBS/Lint even if the configuration enables RBS/Lint. But we need to enable the RBS department explicitly.
It sounds a bug of RuboCop, but I think it's ok to configure it in our configuration file 👍

Copy link

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.

@ksss ksss merged commit 7a0046f into ruby:main Dec 24, 2024
6 checks passed
@ksss ksss deleted the rubocop-on-rbs branch December 24, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants