Skip to content

Conversation

@cho-m
Copy link
Member

@cho-m cho-m commented Dec 3, 2025

  • 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 lgtm (style, typechecking and tests) with your changes locally?

This updates ensure_executable! return to drop nil and uses the with_env return so that Sorbet can infer the type.

Also search all guesses for ensure_executable! prior to installing formula.


Change is done as usually better to avoid T.must as it adds some overhead for all users (as an extra function call even when Sorbet is disabled for non-developers) and is often a sign that code needs refactoring

This updates `ensure_executable!` return to drop nil and uses the
`with_env` return so that Sorbet can infer the type.

Also search all guesses for `ensure_executable!` prior to installing
formula.
Copilot AI review requested due to automatic review settings December 3, 2025 16:57
Copy link
Contributor

Copilot AI left a comment

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 improves type safety in the attestation verification code by eliminating T.must assertions. It updates ensure_executable! to return a non-nilable Pathname type, allowing Sorbet to infer types correctly through the with_env block return value. The implementation is also improved to search all potential executable paths before attempting formula installation.

Key changes:

  • Updated ensure_executable! signature to return Pathname instead of T.nilable(Pathname)
  • Refactored implementation to use find(&:exist?) to search all candidate paths before installing
  • Removed T.must assertion in gh_executable by leveraging with_env return value

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
Library/Homebrew/extend/kernel.rb Updated ensure_executable! signature and implementation to return non-nilable Pathname and search all paths before installing; fixed comment grammar
Library/Homebrew/attestation.rb Removed T.must assertion by using with_env return value directly

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

@cho-m cho-m added this pull request to the merge queue Dec 4, 2025
Merged via the queue into main with commit eef8ca2 Dec 4, 2025
41 checks passed
@cho-m cho-m deleted the attestation-no-must branch December 4, 2025 00:48
@MikeMcQuaid
Copy link
Member

Nice cleanup!

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.

4 participants