-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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 Homebrew::Upgrade #14628
Conversation
Review period will end on 2023-02-15 at 19:10:57 UTC. |
@@ -209,7 +209,7 @@ def install_formula(formula_installer, upgrade:) | |||
ensure | |||
# restore previous installation state if build failed | |||
begin | |||
linked_kegs.each(&:link) if linked_kegs.present? && !f.latest_version_installed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The f
here always results in a NameError
here, afaitct. I believe this was a bug that was introduced in the beginning: https://github.com/Homebrew/brew/pull/12024/files#diff-338a6a0ff7eb3b254fa3b6ca7d9225e7e283521c05acbdcda7f4eb8dc996bc05R206
(You should be able to replicate this by removing the rescue
and running test/cmd/install_spec
.)
However: 1) it wasn't covered by tests (it seems linked_kegs
is always present?
), 2) it's wrapped in a bare rescue
(which I would discourage, but I don't have sufficient context here to know how to better target it), and 3) the file was untyped.
formula
is my best guess as to the original intent, and I've rearranged things a bit so that it's covered in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @dduugg!
Review period skipped due to |
677e12a
to
7fb87ab
Compare
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This enabled typing in the
Homebrew::Upgrade
module