Skip to content

Conversation

@cho-m
Copy link
Member

@cho-m cho-m commented Dec 16, 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?

Part of

Copilot AI review requested due to automatic review settings December 16, 2025 00:24
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 upgrades the linkage_checker.rb file to Sorbet's typed: strict mode as part of a broader effort tracked in issue #17297. The changes add comprehensive type annotations and make necessary code adjustments to satisfy strict type checking requirements while maintaining the existing functionality.

Key changes:

  • Upgraded from typed: true to typed: strict with complete type signatures for all methods and attributes
  • Added explicit type annotations using T.let for all instance variables with appropriate generic types
  • Refactored display_items method to use array operations instead of string concatenation, improving readability and removing dead code

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

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 so far! Most of the T.must though don't seem to be guaranteed to be the case?

cho-m and others added 4 commits December 16, 2025 10:51
This allows using standard Ruby for flow control typing rather than
having to use T.must to express that nil formula was checked by caller.
@cho-m
Copy link
Member Author

cho-m commented Dec 16, 2025

Most of the T.must though don't seem to be guaranteed to be the case?

Added commits to remove some of the T.must. They should have worked based on existing code, but cleaner to refactor in a way that Sorbet can track types.

The ones currently left are:

  1. String <=> String - will always return an Integer but Sorbet can't narrow the return type. T.must is one option. Could alternatively do .to_i. Both should perform similarly as they will require a function call to essentially a no-op.
  2. .split("/").last should not be nil for reasonable formula names. Would require something unexpected like an empty name which would be a bug elsewhere
    brew(main):001> "".split("/").last
    => nil
    brew(main):002> "/".split("/").last
    => nil
    An alternative is .split("/").fetch(-1) if we just want to avoid nil return type
    brew(main):003> "/".split("/").fetch(-1)
    (brew):3:in 'Array#fetch': index -1 outside of array bounds: 0...0 (IndexError)
    I don't think we want to do next if name.nil? as that should be unreachable code so we won't be able to hit it as part of test code coverage.

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