Skip to content

Convert Tab attributes to keyword arguments - #23430

Merged
MikeMcQuaid merged 3 commits into
Homebrew:mainfrom
hyuraku:tab-kwargs
Aug 4, 2026
Merged

Convert Tab attributes to keyword arguments#23430
MikeMcQuaid merged 3 commits into
Homebrew:mainfrom
hyuraku:tab-kwargs

Conversation

@hyuraku

@hyuraku hyuraku commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

AbstractTab#initialize took a hash and assigned attributes via instance_variable_set, so Sorbet checked nothing — a typo (homebrew_versoin) or a wrong type (time: "yesterday") was silently ignored and only surfaced later. Subclasses also needed T.let(nil, ...) for every ivar just to give Sorbet a type. A TODO already asked for this change.

AbstractTab, Tab and Cask::Tab now take keyword arguments:

  • Each class reads its own attributes and passes the rest to super.
  • Unknown keys are swallowed by ** and ignored, so receipts from other Homebrew versions (e.g. the removed installed_as_dependency) still load.
  • The two JSON callers symbolize top-level keys only — source and built_on stay string-keyed, since the code reads them as source["tap"].
  • One behaviour change: tabfile is now always a Pathname; it could previously hold a String, which did not match its signature.

Most typing-only T.let(nil, ...) lines are gone, and every attribute is now listed in one place in the signatures. I added two tests first (unknown attributes, installed_on_request_present?); both pass before and after. Refactor, not a bug fix.


  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include Hyperfine benchmarks.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug?
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

AI (Claude Code) assisted in writing the code and tests.
I reviewed the change and ran brew lgtm locally

@MikeMcQuaid MikeMcQuaid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice cleanup, thanks!

@MikeMcQuaid
MikeMcQuaid added this pull request to the merge queue Aug 4, 2026
@MikeMcQuaid
MikeMcQuaid requested a lite review from Copilot August 4, 2026 13:02
@MikeMcQuaid
MikeMcQuaid removed this pull request from the merge queue due to a manual request Aug 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors AbstractTab, Tab, and Cask::Tab to use keyword arguments instead of a free-form attributes hash, improving Sorbet visibility into Tab attribute types while still allowing forward/backward compatibility for receipt loading via keyword splats.

Changes:

  • Convert AbstractTab, Tab, and Cask::Tab initialisers to explicit keyword arguments and move per-class attribute handling into each class.
  • Update JSON/hash call sites to symbolise top-level keys before splatting into keyword arguments.
  • Add/adjust tests to cover unknown attributes and installed_on_request_present?, and update existing tests to use keyword-based construction.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
Library/Homebrew/test/tab_spec.rb Updates Tab construction to kwargs; adds coverage for unknown attributes and installed_on_request_present?; adjusts JSON round-trip tests for kwargs.
Library/Homebrew/test/installed_dependents_spec.rb Updates Tab construction in tests to kwargs to match new initialisers.
Library/Homebrew/test/cask/tab_spec.rb Updates Cask::Tab construction and JSON round-trip test to use kwargs.
Library/Homebrew/tab/tab.rb Refactors Tab#initialize to typed keyword arguments and normalises changed_files to Pathname.
Library/Homebrew/tab.rb Refactors AbstractTab#initialize/constructors to keyword arguments; updates receipt loading to symbolise top-level keys before splatting.
Library/Homebrew/cask/tab.rb Refactors Cask::Tab#initialize to typed keyword arguments and forwards remaining kwargs to AbstractTab.
Library/Homebrew/bottle.rb Updates bottle tab construction to symbolise keys and splat kwargs into Tab.new.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Library/Homebrew/tab.rb Outdated
Comment thread Library/Homebrew/tab/tab.rb
Comment thread Library/Homebrew/cask/tab.rb
@MikeMcQuaid
MikeMcQuaid enabled auto-merge August 4, 2026 13:32
@MikeMcQuaid
MikeMcQuaid added this pull request to the merge queue Aug 4, 2026
Merged via the queue into Homebrew:main with commit 76fc2ff Aug 4, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants