-
-
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
formula: rename installed_prefix
and opt_or_installed_prefix_keg
#8431
formula: rename installed_prefix
and opt_or_installed_prefix_keg
#8431
Conversation
The old names will need to be kept as aliases (for eventual deprecation) in |
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.
Good idea, thanks for this @SeekingMeaning!
Library/Homebrew/formula.rb
Outdated
# @private | ||
def installed_version | ||
Keg.new(installed_prefix).version | ||
def installed_version_if_latest |
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.
Hmm, not huge on this name but can't think of anything better.
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.
Question: When would this method be preferred over using latest_version_installed?
and/or opt_or_installed_prefix_keg
?
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.
When you specifically want it to fail/return nil
if it's an outdated version. If there's minimal/no usage: it could be removed entirely.
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.
Once #8548 and Homebrew/homebrew-core#59988 are merged, brew
and homebrew-core
shouldn't be using installed_version
anymore but it's possible that it's used in other taps
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.
In that case I reckon keep this name as-is and deprecate it so we can remove it eventually instead.
ec06477
to
3fe31d7
Compare
Currently depends on #8432 |
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.
This is a nice cleanup. Let's roll #8432 into here so they can be compared/combined. Would be good to audit the existing latest_
, any_
, installed_
prefix methods and replace all installed_
with a latest_
or any_
variant depending on what they handle.
Library/Homebrew/formula.rb
Outdated
# @private | ||
def installed_version | ||
Keg.new(installed_prefix).version | ||
def installed_version_if_latest |
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.
In that case I reckon keep this name as-is and deprecate it so we can remove it eventually instead.
3fe31d7
to
f366280
Compare
installed_prefix
and installed_version
installed_prefix
and opt_or_installed_version
installed_prefix
and opt_or_installed_version
installed_prefix
and opt_or_installed_version
installed_prefix
and opt_or_installed_version
installed_prefix
and opt_or_installed_prefix_keg
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.
Fantastic work here, thanks @SeekingMeaning!
brew style
with your changes locally?brew tests
with your changes locally?The names
installed_prefix
andinstalled_version
are misnomers asinstalled_prefix
returns the prefix for the latest version of the formula, regardless of what versions may already be installed.Proposed renames:
installed_prefix
→latest_installed_prefix
opt_or_installed_prefix_keg
→any_installed_keg