Replace some T.nilable(T::Boolean) - #23350
Conversation
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks! Hold for now until you've done audit and ideally @samford and others have chimed in.
| show_output: T.nilable(T::Boolean), | ||
| show_error: T.nilable(T::Boolean), |
There was a problem hiding this comment.
I'm pretty sure these cases (CC @samford) as specifically tri-state to differentiate between:
- set true by caller
- set false by caller
- unset by caller
I don't think this tri-state is likely needed/desired in literally every case you've modified here but it would be good to do a careful audit of each one to consider that.
There was a problem hiding this comment.
Pull request overview
This PR tightens Sorbet type signatures by replacing several T.nilable(T::Boolean) usages with stricter boolean types and corresponding default values/return contracts, aiming to reduce unnecessary tri-state boolean semantics across Homebrew internals.
Changes:
- Tighten boolean keyword-argument and attribute signatures across utils, livecheck, tab, context, and auditing/installer code.
- Adjust method defaults/return values to match stricter boolean types (and update a few call-site conditionals accordingly).
- Update specs to reflect the new typing/argument expectations.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Library/Homebrew/utils/pypi.rb | Tightens boolean params/return types for PyPI resource updating and adjusts early returns. |
| Library/Homebrew/utils/curl.rb | Tightens boolean params for curl helpers and changes debug/verbose defaults. |
| Library/Homebrew/utils/cpan.rb | Tightens boolean params/return type for CPAN resource updating. |
| Library/Homebrew/test/utils/curl_spec.rb | Removes coverage for passing nil where show_output is now a boolean. |
| Library/Homebrew/test/livecheck_spec.rb | Updates expectations for invalid compressed/homebrew_curl arguments. |
| Library/Homebrew/tab/tab.rb | Makes poured_from_bottle/built_as_bottle non-nilable booleans and adjusts initialization. |
| Library/Homebrew/tab.rb | Makes loaded_from_api/loaded_from_internal_api non-nilable booleans and adjusts initialization. |
| Library/Homebrew/livecheck.rb | Narrows accepted option types for compressed/homebrew_curl and removes explicit validation. |
| Library/Homebrew/git_repository.rb | Tightens return type of default_origin_branch? to boolean. |
| Library/Homebrew/formula_installer.rb | Tightens fresh_install? return type to boolean. |
| Library/Homebrew/formula_auditor.rb | Tightens boolean params and provides explicit boolean defaults. |
| Library/Homebrew/extend/os/mac/formula_installer.rb | Tightens fresh_install? return type to boolean in macOS extension. |
| Library/Homebrew/dev-cmd/bump-formula-pr.rb | Adjusts resource-checked condition to treat falsy values uniformly. |
| Library/Homebrew/dev-cmd/bottle.rb | Changes sudo_purge signature and return behaviour. |
| Library/Homebrew/context.rb | Makes context booleans non-nilable and simplifies predicate methods. |
Comments suppressed due to low confidence (2)
Library/Homebrew/livecheck.rb:215
- The removal of explicit validation for
compressed/homebrew_curlchanges runtime behaviour when Sorbet runtime checks are disabled (the default;T.sigis a no-op). Calls likecompressed: trueorhomebrew_curl: falsewill now silently apply invalid values instead of raising, which also makes the updated spec expectations unreliable.
raise ArgumentError, "Only use `post_form` or `post_json`, not both" if post_form && post_json
@options.compressed = compressed unless compressed.nil?
@options.cookies = cookies unless cookies.nil?
@options.header = header unless header.nil?
@options.homebrew_curl = homebrew_curl unless homebrew_curl.nil?
Library/Homebrew/dev-cmd/bottle.rb:321
sudo_purgeis now declared assig { void }but returns booleans (falseor the result ofsystem). Either the sig should return a boolean or the method should returnnilwhen disabled, to match the signature and avoid confusing call sites.
sig { void }
def sudo_purge
return unless ENV["HOMEBREW_BOTTLE_SUDO_PURGE"]
system "/usr/bin/sudo", "--non-interactive", "/usr/sbin/purge"
馃挕 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f00ba56 to
f462ee4
Compare
f462ee4 to
8872428
Compare
6d0106d to
513604a
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
ade94d4 to
5729ce3
Compare
Most of the times nilable booleans are useless as `nil` and `false` serve the same role Signed-off-by: botantony <antonsm21@gmail.com>
5729ce3 to
85e1272
Compare
Most of the times nilable booleans are useless as
nilandfalseserve the same rolebrewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?