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

Jump to bottom

Repo knowledgeable bump formula pr #6538

Closed
wants to merge 3 commits into from
Closed

Repo knowledgeable bump formula pr #6538

wants to merge 3 commits into from

Conversation

morganrconnolly
Copy link

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

@morganrconnolly morganrconnolly mentioned this pull request Oct 5, 2019
Closed
@issyl0
Copy link
Member

issyl0 commented Oct 12, 2019

Thanks for starting on this PR, @morganrconnolly. I’ll take a look at it this weekend!

@issyl0
Copy link
Member

issyl0 commented Oct 12, 2019

Thanks for starting on this!

I found your comment on the original issue and I'll answer it here. I think it's a good idea to get the commands running locally for you so you can test your changes, as I've noticed some syntax issues.

I'm not sure what the appropriate development workflow is for brew commands, e.g. how do modify a command, rebuild brew and then test the modified command. Is there any documentation about this you can point me towards?

There are docs on where to run commands from and how to set up your fork. You can then checkout your existing forked branch and run the code you're editing (brew bump-formula-pr) to test your changes - there are plenty of out of date formula to try running that command with - you can look at the open formula PRs on Homebrew/homebrew-core for inspiration.

Do you run Homebrew on macOS or Linux? If macOS, you'll probably want to test the functionality in our Linux-based Docker container via docker pull homebrew/brew and the above steps.

If you get stuck, or need more advice, do comment here and we can work through it. I'm happy to help! 😄

MikeMcQuaid
MikeMcQuaid reviewed Oct 12, 2019
View reviewed changes
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 for the PR! Have you been able to test this locally? If not, do you need some help figuring out how to do that?

Library/Homebrew/dev-cmd/bump-formula-pr.rb
@@ -297,7 +297,19 @@ def bump_formula_pr
ohai "git checkout --quiet -"
ohai "create pull request with GitHub API"
else

#determine if this formula has tap linuxbrew-core
Copy link
Member

Choose a reason for hiding this comment

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

Can skip (all) your comments as they describe what the code is doing rather than why and generally we favour using variable naming to describe what the code is doing.

Library/Homebrew/dev-cmd/bump-formula-pr.rb
if formula.tap.full_name.includes? "linuxbrew-core"
#try to identify formula in homebrew core
begin
linuxbrew_core_formula = Formulary.factory(formula.gsub('linuxbrew-core','homebrew-core'))
Copy link
Member

Choose a reason for hiding this comment

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

formula.full_name.gsub

Library/Homebrew/dev-cmd/bump-formula-pr.rb
linuxbrew_core_formula = Formulary.factory(formula.gsub('linuxbrew-core','homebrew-core'))
#if there's ambiguity print an informative message
rescue TapFormulaAmbiguityError
odie "Unable to identify which homebrew-core formula corresponds to this linuxbrew formula. Please open PR against homebrew-core for corresponding formula"
Copy link
Member

Choose a reason for hiding this comment

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

When does this occur?

Copy link
Member

Choose a reason for hiding this comment

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

Linux specific formulae I would say.

Library/Homebrew/dev-cmd/bump-formula-pr.rb
@@ -297,7 +297,19 @@ def bump_formula_pr
ohai "git checkout --quiet -"
ohai "create pull request with GitHub API"
else

#determine if this formula has tap linuxbrew-core
if formula.tap.full_name.includes? "linuxbrew-core"
Copy link
Member

Choose a reason for hiding this comment

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

if OS.linux? && formula.tap.core_tap?

@dawidd6
Copy link
Member

dawidd6 commented Nov 5, 2019

After thinking about it, I would say we need to:

  1. Fetch homebrew-core
  2. Check if formula exists in homebrew-core (is not a linux-specific one)
  3. Make a new branch from homebrew-core master
  4. Replace the formula tap name from linuxbrew-core to homebrew-core
  5. Proceed

@dawidd6 dawidd6 mentioned this pull request Nov 10, 2019
Merged
6 tasks
@lock lock bot added the outdated PR was locked due to age label Jan 1, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid left review comments

@dawidd6 dawidd6 dawidd6 left review comments

Assignees
No one assigned
Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@morganrconnolly @issyl0 @dawidd6 @MikeMcQuaid