Skip to content

Keep only the parts of rubocop.rbi that we actually use #20193

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 7, 2025
Merged

Keep only the parts of rubocop.rbi that we actually use #20193

merged 1 commit into from
Jul 7, 2025

Conversation

issyl0
Copy link
Member

@issyl0 issyl0 commented Jun 29, 2025 •
edited
Loading

  • 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?
  • Fixes Reduce size of rubocop.rbi #19146.
  • This file was massive - over 60k lines and we had to bump the file size limit for pushes to the repo!
  • This was because by default Tapioca, when it encounters a require "rubocop" during RBI generation, loads all of the cops ever because they're all classes inside RuboCop::Cop.
  • There wasn't an easy way to control this at Tapioca generation time (we tried), so now we parse the generated RBI file and delete classes and method definitions that we don't use.
  • I regenerated the RBIs (brew tc --update rubocop) and added new things to the allowlist until Sorbet came back green.
  • Now the file is ~7k lines and 240K - much better!

@issyl0 issyl0 requested a review from dduugg June 29, 2025 18:40
@issyl0 issyl0 force-pushed the skinny-rubocop-rbi branch from f39d098 to 9a02120 Compare June 29, 2025 19:01
dduugg
dduugg reviewed Jun 29, 2025
View reviewed changes
Copy link
Member

@dduugg dduugg left a comment

Choose a reason for hiding this comment

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

🤩 Looks promising! I left some initial comments/questions. I'd also suggest moving the logic outside of typecheck.rb and (ofc) adding tests.

Bo98
Bo98 approved these changes Jun 30, 2025
View reviewed changes
Copy link
Member

@Bo98 Bo98 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great, though I do wonder if it's really worth adding 150 lines of code to maintain just to save a few hundred KB in an already autogenerated file? Can it be any simpler even if it makes the RBI a little bigger? No worries if not, just wanted to ask.

@issyl0 issyl0 force-pushed the skinny-rubocop-rbi branch 2 times, most recently from fcc37e7 to 52254ed Compare July 3, 2025 23:17
@issyl0
Copy link
Member Author

issyl0 commented Jul 3, 2025 •
edited
Loading

Can it be any simpler even if it makes the RBI a little bigger?

I tried, @Bo98. I tried making it a Tapioca compiler like we have prior art for rather than something just shoved on the end of brew tc --update, but I didn't have much success so kept it this way - and I'd prefer to just do the thing required to not bloat our repo any more. I did add tests though, if that makes you feel any better.

@issyl0
Keep only the parts of rubocop.rbi that we actually use
836d852 
- This file was _massive_ - over 60k lines and we had to bump the file
  size limit for pushes to the repo!
- This was because by default Tapioca, when it encounters a
  `require "rubocop"` during RBI generation, loads all of the cops ever
  because they're all classes inside `RuboCop::Cop`.
- There wasn't an easy way to control this at Tapioca generation time
  (we tried), so now we parse the generated RBI file and delete classes
  and method definitions that we don't use.
- I regenerated the RBIs (`brew tc --update rubocop`) and added new
  things to the allowlist until Sorbet came back green.
- Now the file is ~7k lines and 240K - much better!
@issyl0 issyl0 force-pushed the skinny-rubocop-rbi branch from 52254ed to 836d852 Compare July 3, 2025 23:27
MikeMcQuaid
MikeMcQuaid approved these changes Jul 7, 2025
View reviewed changes
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.

Looks good when working!

@MikeMcQuaid
Copy link
Member

@issyl0 Is this ready to 🚢?

@issyl0
Copy link
Member Author

issyl0 commented Jul 7, 2025

It is - I was waiting for a follow-up review from the people who requested changes (and also distracted).

@issyl0 issyl0 added this pull request to the merge queue Jul 7, 2025
@MikeMcQuaid
Copy link
Member

@issyl0 woohoo great work!

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 7, 2025
@issyl0
Copy link
Member Author

issyl0 commented Jul 7, 2025

wat

refusing to atomically replace "/home/linuxbrew/.linuxbrew/bin/.brew.formatted~" with a regular file as it is not one already
Failed to run `shfmt` for file "/home/linuxbrew/.linuxbrew/bin/brew".
shfmt.sh: Failed to format file "/home/linuxbrew/.linuxbrew/bin/brew". Formatter exited with code 2 (`shfmt` failed).

@issyl0 issyl0 added this pull request to the merge queue Jul 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 7, 2025
@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Jul 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 7, 2025
@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Jul 7, 2025
Merged via the queue into main with commit 5ec756e Jul 7, 2025
35 checks passed
@MikeMcQuaid MikeMcQuaid deleted the skinny-rubocop-rbi branch July 7, 2025 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dduugg dduugg dduugg left review comments

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

@Bo98 Bo98 Bo98 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reduce size of rubocop.rbi
4 participants
@issyl0 @MikeMcQuaid @dduugg @Bo98