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

deprecate_disable: support optional replacement parameter #18733

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

alebcay
Copy link
Member

@alebcay alebcay commented Nov 7, 2024

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

Addresses #18294.

A replacement may be specified like so:

deprecate! date: "2024-01-10", because: :repo_removed, replacement: "ffmpeg"

which yields something like

$ HOMEBREW_NO_INSTALL_FROM_API=1 brew info alac
==> alac: stable 0.2.0 (bottled)
Basic decoder for Apple Lossless Audio Codec files (ALAC)
https://web.archive.org/web/20150319040222/craz.net/programs/itunes/alac.html
Deprecated because it has a removed upstream repository! It will be disabled on 2025-01-10.
Consider replacing it with:
    brew install ffmpeg
Not installed
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/a/alac.rb
License: MIT
==> Analytics
install: 8 (30 days), 34 (90 days), 110 (365 days)
install-on-request: 8 (30 days), 34 (90 days), 110 (365 days)
build-error: 0 (30 days)

and, for disabled:

disable! date: "2024-08-03", because: :unmaintained, replacement: "gcalcli"

which yields something like

$ HOMEBREW_NO_INSTALL_FROM_API=1 brew info gcal
==> gcal: stable 4.1 (bottled)
Program for calculating and printing calendars
https://www.gnu.org/software/gcal/
Disabled because it is not maintained upstream! It was disabled on 2024-08-03.
Consider replacing it with:
    brew install gcalcli
Not installed
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/g/gcal.rb
License: GPL-3.0-or-later
==> Dependencies
Build: texinfo ✔
==> Analytics
install: 0 (30 days), 2 (90 days), 179 (365 days)
install-on-request: 0 (30 days), 2 (90 days), 179 (365 days)
build-error: 0 (30 days)

  • Replacements are added as an optional keyword argument. The message and behavior when no replacement is provided should remain the same.
  • I made an effort to include the replacement in the API JSON as well, but I'm not too familiar. Would appreciate if someone can confirm that the changes made there are sufficient.
  • The replacement is just a string for now, which is shown as an argument to the brew install help text. Note that there is no distinction between the string argument as a formula or cask; there is no validation on the value of the string.
  • I am aware that formula: allow excluding deprecate! reason when disable! exists #18661 is open and this may need to be refactored after that is merged (or vice versa).

@reitermarkus
Copy link
Member

reitermarkus commented Nov 7, 2024

I made an effort to include the replacement in the API JSON as well

I'd suggesst this be an array in the API JSON, in case we want to add support for multiple replacements to the DSL.

Maybe it should be called alternatives as well in that case.

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.

One style/prose nit and then should be good to 🚢!

Library/Homebrew/deprecate_disable.rb Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

I'd suggesst this be an array in the API JSON, in case we want to add support for multiple replacements to the DSL.

I would very much like us to avoid specifying multiple replacements for a single formula (and therefore making this an array).

It's much less useful to say "to replace this: go check out multiple other projects and decide" than "to replace this: we have decided X is the best option".

@alebcay alebcay force-pushed the deprecate-disable-replacement branch from 8f7cb94 to 215fc85 Compare November 8, 2024 20:49
@alebcay
Copy link
Member Author

alebcay commented Nov 8, 2024

Changed the replacement message to use a heredoc and reduced indentation to two spaces. Note that the heredoc adds a trailing newline compared to the previous inline string.

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.

Thanks @alebcay!

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