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

untap: add --force switch #9489

Merged
merged 1 commit into from Dec 15, 2020
Merged

untap: add --force switch #9489

merged 1 commit into from Dec 15, 2020

Conversation

dawidd6
Copy link
Member

@dawidd6 dawidd6 commented Dec 9, 2020

Another take on: #9422

Changes in comparison to previous PR:

  • if HOMEBREW_DEVELOPER env variable is set and there are tap formulae or casks installed, print warning instead of dying

  • assing formulae and casks name to variable, so we avoid duplicated logic

  • 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?

  • Have you successfully run brew man locally and committed any changes?


@BrewTestBot
Copy link
Member

Review period will end on 2020-12-10 at 21:52:38 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 9, 2020
@dawidd6 dawidd6 force-pushed the untap-force branch 2 times, most recently from 583c622 to 717479d Compare December 9, 2020 21:56
@reitermarkus
Copy link
Member

if HOMEBREW_DEVELOPER env variable is set and there are tap formulae or casks installed, print warning instead of dying

I find this logic is inverse to anything else switched by HOMEBREW_DEVELOPER. If I am a HOMEBREW_DEVELOPER, I want a hard failure instead of a warning so I, as a developer, can submit a PR to fix the error.

@reitermarkus
Copy link
Member

We should make this:

  1. A warning for everyone.
  2. A failure for HOMEBREW_DEVELOPERs, a warning for everyone else.
  3. A failure for everyone.

installed_tap_formulae = Formula.installed.select { |formula| formula.tap == tap }
installed_tap_casks = Cask::Caskroom.casks.select { |cask| cask.tap == tap }

if installed_tap_formulae.length.positive? || installed_tap_casks.length.positive?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if installed_tap_formulae.length.positive? || installed_tap_casks.length.positive?
if installed_tap_formulae.any? || installed_tap_casks.any?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if installed_tap_formulae.length.positive? || installed_tap_casks.length.positive?
if installed_tap_formulae.present? || installed_tap_casks.present?

installed_tap_formulae = Formula.installed.select { |formula| formula.tap == tap }
installed_tap_casks = Cask::Caskroom.casks.select { |cask| cask.tap == tap }

if installed_tap_formulae.length.positive? || installed_tap_casks.length.positive?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if installed_tap_formulae.length.positive? || installed_tap_casks.length.positive?
if installed_tap_formulae.present? || installed_tap_casks.present?

Co-authored-by: Eric Knibbe <enk3@outlook.com>
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 11, 2020
@BrewTestBot
Copy link
Member

Review period ended.

Copy link
Member

@reitermarkus reitermarkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeMcQuaid
Copy link
Member

  • A warning for everyone.
  • A failure for HOMEBREW_DEVELOPERs, a warning for everyone else.
  • A failure for everyone.

I disagree. The way this is implemented in the PR is consistent with how brew uninstall behaves wrt. dependencies.

@dawidd6 dawidd6 merged commit d73fc48 into Homebrew:master Dec 15, 2020
@dawidd6 dawidd6 deleted the untap-force branch December 15, 2020 17:48
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 15, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 15, 2021
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.

None yet

4 participants