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

bump-formula-pr: restore formula if duplicate PR exists #8011

Merged
merged 1 commit into from Jul 16, 2020

Conversation

SeekingMeaning
Copy link
Contributor

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

I figured this made sense since formulae are restored for bumps that are downgrades or don't actually change the version: https://github.com/Homebrew/brew/blob/master/Library/Homebrew/dev-cmd/bump-formula-pr.rb#L332-L344

@maxim-belkin
Copy link
Member

I wonder if bump-formula-pr should even do something if there is an open PR already? Shall it, perhaps, do this check early and bail out if there is an open pull request?

@SeekingMeaning
Copy link
Contributor Author

SeekingMeaning commented Jul 15, 2020

Yes, we can do the open pull requests check early, although the closed PRs check will have to stay since it relies on knowing what's the latest version

Not sure if I should make this change in this PR or another

@jonchang
Copy link
Contributor

It's fine to make it in a new pull request. This is handy, thanks!

@jonchang jonchang merged commit 9c7f73a into Homebrew:master Jul 16, 2020
@SeekingMeaning SeekingMeaning deleted the bump-duplicate branch July 16, 2020 08:10
@maxim-belkin
Copy link
Member

backup_file is a slightly misleading variable name -- it's actually old_contents rather than backup_file (which we could mv instead of having to .atomic_write it). Changing the name is easy so maybe @SeekingMeaning could do that in a new PR (I don't want to get ownership of lines just for changing the variable name).

Also, we need to make sure backup_file (or old_contents) is defined for dry-runs as well.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 24, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 24, 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.

None yet

4 participants