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

cmd/update-reset: improve arg parsing #14667

Merged
merged 4 commits into from Feb 28, 2023

Conversation

carlocab
Copy link
Member

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

Currently, doing brew update-reset homebrew/core does nothing (not
even return an error). If you want to update-reset a given tap, you
must do (the equivalent of)

brew update-reset "$(brew --repository owner/tap_name)"

This isn't very intuitive, so let's do a bit more work in argument
parsing so that the user can just pass a tap name instead of a path to a
tap.

Passing a path to a tap is also still supported.

Currently, doing `brew update-reset homebrew/core` does nothing (not
even return an error). If you want to `update-reset` a given tap, you
must do (the equivalent of)

    brew update-reset "$(brew --repository owner/tap_name)"

This isn't very intuitive, so let's do a bit more work in argument
parsing so that the user can just pass a tap name instead of a path to a
tap.

Passing a path to a tap is also still supported.
@BrewTestBot
Copy link
Member

Review period will end on 2023-02-17 at 16:02:18 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 16, 2023
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.

I don't think we should add this. This logic will not behave the same as the tap naming logic in Ruby and is likely to further confuse. Instead, let's error out if we get no valid arguments.

@carlocab
Copy link
Member Author

This logic will not behave the same as the tap naming logic in Ruby and is likely to further confuse.

I don't follow. It does use the same tap naming logic as in Ruby. If you mean it allows you to refer to a tap as homebrew/homebrew-core instead of just homebrew/core, then that can be easily removed.

@MikeMcQuaid
Copy link
Member

It does use the same tap naming logic as in Ruby.

It's not identical logic and it's not identical code.

I think this is much more easily and reliably fixed by updating documentation and error messaging rather than trying to parse a new format of input consistently.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 17, 2023
@BrewTestBot
Copy link
Member

Review period ended.

@carlocab carlocab changed the title cmd/update-reset: accept tap names as arguments cmd/update-reset: improve arg parsing Feb 28, 2023
@carlocab
Copy link
Member Author

Instead, let's error out if we get no valid arguments.

Did this partially -- we now error out if any argument is invalid. We can continue if at least one argument is valid, but given that this update-reset is destructive it seems safer to err on not doing anything.

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.

Nice improvements, thanks @carlocab!

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.

@carlocab
Copy link
Member Author

Oops, looks like this broke: https://github.com/Homebrew/brew/actions/runs/4293038788/jobs/7480240578#step:8:61

Well, now we know that command was quietly not doing anything! 😄

This command was being called with the wrong relative path, so it
silently did nothing. Now that `update-reset` errors out from invalid
arguments, we know that running `update-reset` here is not needed.
@@ -103,8 +103,6 @@ jobs:
brew tap homebrew/portable-ruby
brew tap homebrew/services

brew update-reset Library/Taps/homebrew/homebrew-bundle
Copy link
Member Author

Choose a reason for hiding this comment

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

I've now removed it. I can put it back with the correct path, but given that this has been a silent no-op for a while now I suspect we don't need it.

Copy link
Member

Choose a reason for hiding this comment

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

@carlocab Yup, all good, thanks!

@carlocab carlocab merged commit 1201f18 into Homebrew:master Feb 28, 2023
@carlocab carlocab deleted the update-reset-tap-names branch February 28, 2023 14:09
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 31, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2023
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

3 participants