Skip to content
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

Merged

Conversation

SeekingMeaning
Copy link
Contributor

@SeekingMeaning SeekingMeaning commented Aug 21, 2020

  • 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 style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The names installed_prefix and installed_version are misnomers as installed_prefix returns the prefix for the latest version of the formula, regardless of what versions may already be installed.

Proposed renames:
installed_prefixlatest_installed_prefix
opt_or_installed_prefix_kegany_installed_keg

@Bo98
Copy link
Member

Bo98 commented Aug 21, 2020

The old names will need to be kept as aliases (for eventual deprecation) in compat.

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.

Good idea, thanks for this @SeekingMeaning!

# @private
def installed_version
Keg.new(installed_prefix).version
def installed_version_if_latest
Copy link
Member

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.

Copy link
Contributor Author

@SeekingMeaning SeekingMeaning Aug 27, 2020

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?

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

@SeekingMeaning
Copy link
Contributor Author

Currently depends on #8432

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.

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.

# @private
def installed_version
Keg.new(installed_prefix).version
def installed_version_if_latest
Copy link
Member

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.

@SeekingMeaning SeekingMeaning force-pushed the formula/installed_prefix branch from 3fe31d7 to f366280 Compare September 1, 2020 16:54
@SeekingMeaning SeekingMeaning changed the title formula: rename installed_prefix and installed_version formula: rename installed_prefix and opt_or_installed_version Sep 1, 2020
@SeekingMeaning SeekingMeaning changed the title formula: rename installed_prefix and opt_or_installed_version formula: rename installed_prefix and opt_or_installed_version Sep 1, 2020
@SeekingMeaning SeekingMeaning changed the title formula: rename installed_prefix and opt_or_installed_version formula: rename installed_prefix and opt_or_installed_prefix_keg Sep 1, 2020
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.

Fantastic work here, thanks @SeekingMeaning!

@MikeMcQuaid MikeMcQuaid merged commit 5e5dabc into Homebrew:master Sep 1, 2020
@SeekingMeaning SeekingMeaning deleted the formula/installed_prefix branch September 1, 2020 19:53
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 14, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants