brew icon indicating copy to clipboard operation
brew copied to clipboard

Add more flags to `brew bump --open-pr` to match `bump-*-pr`

Open nihaals opened this issue 2 years ago • 13 comments

Provide a detailed description of the proposed feature

See: #13166 and https://github.com/Homebrew/brew/issues/13166#issuecomment-1104954203


On Homebrew 3.4.7:

brew bump --help
Usage: brew bump [options] [formula|cask ...]

Display out-of-date brew formulae and the latest version available. If the
returned current and livecheck versions differ or when querying specific
formulae, also displays whether a pull request has been opened with the URL.

      --full-name                  Print formulae/casks with fully-qualified
                                   names.
      --no-pull-requests           Do not retrieve pull requests from GitHub.
      --formula, --formulae        Check only formulae.
      --cask, --casks              Check only casks.
      --open-pr                    Open a pull request for the new version if
                                   there are none already open.
      --limit                      Limit number of package results returned.
      --start-with                 Letter or word that the list of package
                                   results should alphabetically follow.
  -d, --debug                      Display any debugging information.
  -q, --quiet                      Make some output more quiet.
  -v, --verbose                    Make some output more verbose.
  -h, --help                       Show this message.
brew bump-cask-pr --help
Usage: brew bump-cask-pr [options] cask

Create a pull request to update cask with a new version.

A best effort to determine the SHA-256 will be made if the value is not
supplied by the user.

  -n, --dry-run                    Print what would be done rather than doing
                                   it.
      --write-only                 Make the expected file modifications without
                                   taking any Git actions.
      --commit                     When passed with --write-only, generate a
                                   new commit after writing changes to the cask
                                   file.
      --no-audit                   Don't run brew audit before opening the PR.
      --online                     Run brew audit --online before opening the
                                   PR.
      --no-style                   Don't run brew style --fix before opening
                                   the PR.
      --no-browse                  Print the pull request URL instead of opening
                                   in a browser.
      --no-fork                    Don't try to fork the repository.
      --version                    Specify the new version for the cask.
      --message                    Append message to the default pull request
                                   message.
      --url                        Specify the URL for the new download.
      --sha256                     Specify the SHA-256 checksum of the new
                                   download.
      --fork-org                   Use the specified GitHub organization for
                                   forking.
  -f, --force                      Ignore duplicate open PRs.
  -d, --debug                      Display any debugging information.
  -q, --quiet                      Make some output more quiet.
  -v, --verbose                    Make some output more verbose.
  -h, --help                       Show this message.

Part of this issue is also documenting what brew bump actually does/how it differs from brew bump-cask-pr. From flags like --no-audit, --no-style, --no-browse and --fork-org, it's fairly clear what bump-cask-pr actually does, whereas bump only gives a high level overview. It isn't clear if it runs style, audit or opens the PR in your browser (or even creates the PR for you vs. taking you to open the PR in your browser). Adding a --dry-run could help with this, and some of these additional flags are exclusive to --open-pr, which might make reading the manual/--help more difficult.

What is the motivation for the feature?

From #13166 (referring to adding a brew bumo-cask-pr --livecheck flag):

Currently if you want to bump a cask's version you need to get the latest version either from the site/app or from brew livecheck/brew bump. Since Homebrew has access to the current latest version already with the livecheck, passing the actual version shouldn't need to be required when bumping. You can currently use brew livecheck --json to make your own function that passes it for you, but this adds complexity for a new user to use it.

How will the feature be relevant to at least 90% of Homebrew users?

From #13166:

This is for users who update casks, but for those users it will make it a faster process for anyone who sees an app update themselves and wants to PR a version bump.

What alternatives to the feature have been considered?

From https://github.com/Homebrew/brew/issues/13166#issuecomment-1104501806:

  • Add livecheck to bump-cask-pr
  • Add more flags to brew bump --open-pr
  • Try to merge the commands in some way

nihaals avatar Apr 21 '22 17:04 nihaals

Internally brew bump --open-pr delegates to brew bump-cask-pr or brew bump-formula-pr if a newer version of the cask or formula exists without an open pull request. You are right in suggesting that it doesn't currently allow you to pass flags to those commands though.

This is the full command that gets called internally when using brew bump --open-pr.

brew bump-[formula|cask]-pr --no-browse --message-"Created by `brew bump`"
--version=[new version] [formula|cask name]

apainintheneck avatar Apr 21 '22 23:04 apainintheneck

Makes sense to me!

MikeMcQuaid avatar Apr 22 '22 13:04 MikeMcQuaid

Since brew bump is delegating to those other commands internally, wouldn't the simplest solution be to just pass those arguments directly to the other command instead of adding more flags to brew bump. Something like the following...

brew bump --open-pr="--no-browse --no-style --fork-org --no-audit"

Then, validation of those arguments would just get handled by the next command. Just a thought.

apainintheneck avatar Apr 24 '22 21:04 apainintheneck

brew bump --open-pr="--no-browse --no-style --fork-org --no-audit"

That does sound like an interesting and simple solution, although it doesn't feel the "smoothest" e.g. breaks shell completions.

nihaals avatar Apr 24 '22 22:04 nihaals

Then, validation of those arguments would just get handled by the next command.

I think it'd be nicer to have the validation and documentation handled by the first command and also to add only the specific arguments here that @nihaals or whoever needs rather than all of them.

MikeMcQuaid avatar Apr 25 '22 08:04 MikeMcQuaid

@nihaals Could you list the specific arguments you'd want added to brew bump?

apainintheneck avatar Jun 08 '22 18:06 apainintheneck

Could you list the specific arguments you'd want added to brew bump?

@apainintheneck I think these make the most sense to me:

  • --no-audit
  • --no-style
  • --no-fork
  • --fork-org

  • --dry-run?
  • --online?
  • --browse (opposite of --no-browse since that's already the default)?

nihaals avatar Jun 09 '22 16:06 nihaals

I'm not sure about the --no-style option because that option is only used with bump-cask-pr and not bump-formula-pr but the other ones sound reasonable.

apainintheneck avatar Jun 09 '22 17:06 apainintheneck

  • --no-audit
  • --no-style

I'd rather we didn't proliferate these too much further.

  • --no-fork
  • --fork-org

Why do you need both of these?

  • --dry-run?

This will be a non-trivial lift to add.

  • --online?

This tool doesn't make sense offline.

MikeMcQuaid avatar Jun 10 '22 14:06 MikeMcQuaid

I agree with the points about the other flags.

  • --no-fork
  • --fork-org

Why do you need both of these?

I thought I ran into an issue where because I used a GitHub token with just public_repo (as I had already forked the repo, added the remote and was using SSH), I was getting authorisation errors and needed to use --no-fork, although I didn't have this issue when using bump --open-pr earlier today.

As for --fork-org, I assume Homebrew uses the owner of the token as the owner of the fork, but there may be a case where someone does want to use an org fork instead of a personal fork. I haven't had a chance to look at the source yet to see how Homebrew looks at existing remotes/what it expects the remote to be called and the logic it uses for checking for existing forks, but if it always assumes a personal fork, then someone might need this flag.

Since I haven't looked at the source yet, I also haven't seen how much --no-fork saves in cases where the fork already exists and the remote is already added, but I assume the flag saves a single API request, which probably doesn't matter for a PAT created just for Homebrew, so it could probably be left.

  • --online?

This tool doesn't make sense offline.

Should --online always be passed to brew bump-[formula|cask]-pr then?

nihaals avatar Jun 13 '22 19:06 nihaals

Should --online always be passed to brew bump-[formula|cask]-pr then?

Probably, yes.

In general, I'd like to see more of these flags replaced with better handling/defaults rather than just passing them through.

MikeMcQuaid avatar Jun 14 '22 07:06 MikeMcQuaid

Hopefully this makes it easier to keep track of each flag:

bump-cask-pr flag Add to bump? Notes
--dry-run Out of scope
--write-only
--commit
--no-audit :x: https://github.com/Homebrew/brew/issues/13171#issuecomment-1152448644
--online :x: https://github.com/Homebrew/brew/issues/13171#issuecomment-1154841214
--no-style :x: https://github.com/Homebrew/brew/issues/13171#issuecomment-1151422766, https://github.com/Homebrew/brew/issues/13171#issuecomment-1152448644
--browse
--no-fork
--fork-org
--force Covered by --no-pull-requests?

nihaals avatar Jun 14 '22 13:06 nihaals

--force isn't covered by --no-pull-requests if you want to open the PR: Error: Invalid usage: Options --no-pull-requests and --open-pr are mutually exclusive.

Which means when it find irrelevant opened PR, you currently can't force it to open one from brew bump.

Also, I don't don't see --version listed, which make brew bump unusable on many throttled formulas, excepted if it can calculate the latest acceptable version in the interval rather than the latest, or if throttled formulas would accept any version not multiple of X where X is the throttle, but any version with an interval of more than X instead.

Porkepix avatar Aug 14 '22 08:08 Porkepix

Passing on this. We'll review a PR but not going to leave it open indefinitely, sorry!

MikeMcQuaid avatar Mar 23 '23 08:03 MikeMcQuaid