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

.rubocop.yml: selectively enable Rails cops #12567

Merged
merged 1 commit into from Dec 16, 2021

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Dec 15, 2021

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Some Rails cops use ActiveRecordHelper which contains a cache checksum that involves checksuming a Rails database schema. This involves an amount of file traversal as it tries to find this schema, which can be slow.

For cached brew style runs on Homebrew/core, disabling these cops speeds up execution time by over 50%.

@Bo98 Bo98 added the critical Critical change which should be shipped as soon as possible. label Dec 15, 2021
@BrewTestBot
Copy link
Member

BrewTestBot commented Dec 15, 2021

Review period ended.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense to me, thanks @Bo98. I wonder if we can just unconditionally disable these Rails cops and only enable those we find specifically useful?

Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

After a quick glance at the Rails cops, I agree that it looks like most can be removed completely. Off the top of my head, I know we definitely want to keep Rails/Blank, Rails/Presence, and Rails/Present. There might be others, but if I had to guess those three are the main reason we use any Rails cops (unless they come by default which might be the case)

@MikeMcQuaid
Copy link
Member

Looks good to me!

After a quick glance at the Rails cops, I agree that it looks like most can be removed completely. Off the top of my head, I know we definitely want to keep Rails/Blank, Rails/Presence, and Rails/Present. There might be others, but if I had to guess those three are the main reason we use any Rails cops (unless they come by default which might be the case)

Rails/ActiveSupportAliases, maybe Rails/Date, Rails/Delegate, Rails/DelegateAllowBlank, maybe Rails/EnvironmentVariableAccess (one day, currently disabled), maybe Rails/Exit, Rails/ExpandedDateRange, Rails/NegateInclude, Rails/SafeNavigation, Rails/SafeNavigationWithBlank look useful, too.

@Bo98 Bo98 changed the title .rubocop.yml: disable slow Rails cops .rubocop.yml: selectively enable Rails cops Dec 16, 2021
@Bo98
Copy link
Member Author

Bo98 commented Dec 16, 2021

We should monitor the changelog in rubocops-rails Dependabot PRs to see what new cops should be added.

@MikeMcQuaid MikeMcQuaid merged commit 64bd95c into Homebrew:master Dec 16, 2021
@MikeMcQuaid
Copy link
Member

Thanks @Bo98!

@Bo98 Bo98 deleted the no-slow-rails-cops branch December 16, 2021 17:12
@github-actions github-actions bot added the outdated PR was locked due to age label Jan 16, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants