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

Enable typing in rubocops #14649

Merged
merged 2 commits into from Feb 17, 2023
Merged

Enable typing in rubocops #14649

merged 2 commits into from Feb 17, 2023

Conversation

dduugg
Copy link
Sponsor Member

@dduugg dduugg commented Feb 15, 2023

  • 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?

This changes enables typing for everything under Library/Homebrew/rubocops.

The most significant change here is probably enabling --enable-experimental-requires-ancestor flag in sorbet. Since the feature "is experimental and might be changed or removed without notice", i've put every use of requires_ancestror in an rbi file, so that the runtime does depend on a potentially breaking change. (However, since sorbet-runtime appears to be vendored, this may be overkill.)

@BrewTestBot
Copy link
Member

Review period will end on 2023-02-16 at 18:40:07 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 15, 2023
@dduugg dduugg force-pushed the type-rubocops branch 2 times, most recently from b8ba494 to 9e550fe Compare February 16, 2023 06:39
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.

Thanks again @dduugg!

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 16, 2023
@BrewTestBot
Copy link
Member

Review period ended.

@dduugg
Copy link
Sponsor Member Author

dduugg commented Feb 16, 2023

This PR revealed an interesting design artifact. It seems FormulaCop is intended to be a sort of abstract super class for formulae cops, but rubocop has been running as a cop itself all along. This wasn't noticed before, because its on_class handler would early exit due to the lack of an audit_formula method.

However, sorbet doesn't allow FormulaCop to invoke audit_formula without it being defined in the class (and sorbet doesn't support respond_to? guards, because it can't check the method param/return types, perform an arity check, etc.). Thus I defined a pseudo-abstract audit_formula method in FormulaCop, which raises a NotImplementedError (I can't just mark the class and method abstract due to how rubocop instantiates Cop subclasses).

I've worked around the issue for now by disabling FormulaCop in .rubocop.yml (its subclasses remain enabled), with a note to convert it to interface. Other solutions could include having audit_formula no-op in FormulaCop rather than raising a NotImplementedError, or creating an interface for the audit_formula method that FormulaCop subclasses are required to implement. I don't like these approaches as much though, because they maintain FormulaCop as Cop for rubocop to run, which I don't think is the intention.

@Bo98
Copy link
Member

Bo98 commented Feb 17, 2023

Yeah I did redo the FormulaCop stuff before to be a Force rather than a Cop but there were a lot of changes to rubocops at the time that it meant conflicts were frequent (and I needed to make changes in other taps because of the slight behavioural changes), plus there was hesitation against such a large refactor. Maybe worth revisiting again at some point on some quiet week.

@MikeMcQuaid MikeMcQuaid merged commit e358035 into Homebrew:master Feb 17, 2023
@dduugg dduugg deleted the type-rubocops branch February 17, 2023 00:28
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants