Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

browse -b flag #258

Closed
wants to merge 8 commits into from

6 participants

@jianli

hub browse -b opens the URL of the pull request associated with your current branch.
hub browse -b foo opens the URL of the pull request associated with branch foo.

This is my first time coding Ruby, so feedback is appreciated!

@jianli jianli referenced this pull request
Closed

pull-request -l flag #255

@mislav
Admin

I'm curious as to why you needed this functionality?

The Ruby code is not bad, although you should be writing tests for new features, but I don't see how this command fits in the workflow.

The other trouble I see with it that it transforms the old pull-request command into something else. The original pull-request command makes a pull requests, while your pull-request -l just prints data. It's an entirely different feature embedded in the same command, which is against unix philosophy.

@jianli

I'm curious as to why you needed this functionality?

This flag has actually been quite useful to me; I have a workflow where I have several feature branches and pull requests active at the same time, so I need a quick way to get the URL of the pull request given a feature branch. For example,

open -a 'Google Chrome' `hub pull-request -l`

There is nothing in the Github web interface which maps feature branches to pull requests, so it's only through the API that this can be achieved.

The other trouble I see with it that it transforms the old pull-request command into something else. The original pull-request command makes a pull requests, while your pull-request -l just prints data. It's an entirely different feature embedded in the same command, which is against unix philosophy.

Would you suggest a new command? I couldn't think of a good name other than pull-request.

My original rationale was for overloading pull-request was this: The original pull-request actually prints the URL as well after successfully creating the pull request. But if you run pull-request for a feature branch which already has one, hub gives the unhelpful "Error creating pull request: Unprocessable Entity (HTTP 422)". So this patch seems to be an improvement in that it actually prints the specific pull request if it exists.

I guess a cleaner way would be to always check that there is an existing pull request right before creating one, though that would have performance implications for the current use case.

@jianli

I wrote some tests.

@mislav
Admin

Everything looks fine. I can see how this can be useful. However I won't pull this just yet because I don't like the fact that we're overloading pull-request.

I have an idea of splitting hub into multiple smaller scripts that follow the unix philosophy. A command that provides the thing you need would fit there.

@jianli

Here's a proposal:

It seems that the current behavior of hub pull-request is

  1. Try to create a pull request. If one currently exists, print a 422 error message.
  2. Otherwise, create a new pull request and print its name.

What if we made the behavior

  1. Check if a pull-request exists. If so, print its name
  2. Otherwise, create a new pull request and print its name

I know this won't solve the issue of overloading pull-request, but it seems more elegant to me.

@barraponto

we already have hub browse, that's what we should overload (i think).

@edsinclair

:+1: for adding this to hub browse:

From the man page:
https://github.com/defunkt/hub/blob/master/man/hub.1#L396-L415

It looks like it belongs there.

@jianli

Ok, I've moved this command to hub browse and updated the PR description up top.

There is (still) a test for this feature. It seems that one or more of hub.1, hub.1.html, hub.1.ronn, and README.md is automatically generated from the others, so I don't know how to proceed with adding documentation in those places.

@mislav
Admin

This is interesting functionality. I'm planning to add a command that lists open pull requests. This seems to belong to this new command, but on the other hand it seems like it belongs to browse as well. I need to think about this.

@emarref

Just wanted to chime in here to say I'm very interested in being able to view the pull request related to a branch also. Looking forward to this feature.

@jianli

I've realized that it's possible to do this by simply navigating to github.com/user/repo/pull/branch_name, which bypasses the API calls made in this PR.

This is what I currently use (zsh function):

function pr
{
    url=`hub browse -u`
    echo ${url/tree/pull}
    open ${url/tree/pull}
}
@mislav
Admin

@jianli Yes I've just discovered this last week as well. It's a great feature. I'm thinking of baking it in the next version of hub, but we still have to decide on the command syntax. I don't want to overload the browse command any more.

@aik099

The browse command is the real time save for me. Thanks.

@mislav
Admin

Closing this PR as it won't apply anymore since we nuked the Ruby implementation and replaced it with Go: #642. Sorry that this hasn't made it in. We plan to improve the browse command in future releases, though.

@mislav mislav closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 69 additions and 1 deletion.
  1. +12 −1 lib/hub/commands.rb
  2. +20 −0 lib/hub/github_api.rb
  3. +37 −0 test/hub_test.rb
View
13 lib/hub/commands.rb
@@ -615,6 +615,9 @@ def push(args)
# $ hub browse
# > open https://github.com/CURRENT_REPO
#
+ # $ hub browse -b BRANCH_NAME
+ # > open https://github.com/CURRENT_REPO/pull/258
+ #
# $ hub browse -- issues
# > open https://github.com/CURRENT_REPO/issues
#
@@ -632,7 +635,15 @@ def browse(args)
dest = args.shift
dest = nil if dest == '--'
- if dest
+ if dest == '-b'
+ branch_name = args.shift || current_branch.short_name
+ pr_url = api_client.get_pullrequest(local_repo.main_project, branch_name)
+ if pr_url
+ next pr_url
+ else
+ abort "Pull request not found for branch #{branch_name}."
+ end
+ elsif dest
# $ hub browse pjhyett/github-services
# $ hub browse github-services
project = github_project dest
View
20 lib/hub/github_api.rb
@@ -118,6 +118,26 @@ def statuses project, sha
res.data
end
+ # Return the pull request corresponding to the current branch
+ def get_pullrequest project, branch_name
+ for state in ['open', 'closed']
+ page = 1
+ res = nil
+ while page == 1 or res.data.length > 0
+ res = get "https://%s/repos/%s/%s/pulls?state=%s&page=%s" %
+ [api_host(project.host), project.owner, project.name, state, page]
+ res.error! unless res.success?
+ res.data.each { |x|
+ if branch_name == x['head']['label'].split(':', 0)[1]
+ return x['html_url']
+ end
+ }
+ page += 1
+ end
+ end
+ nil
+ end
+
# Methods for performing HTTP requests
#
# Requires access to a `config` object that implements:
View
37 test/hub_test.rb
@@ -356,6 +356,30 @@ def test_hub_browse_no_repo
assert_equal "Usage: hub browse [<USER>/]<REPOSITORY>\n", hub("browse")
end
+ def test_hub_browse_branch
+ stub_request(:get, "https://api.github.com/repos/defunkt/hub/pulls?page=1&state=open").
+ to_return(:body =>
+ mock_list_pulls_response({
+ 'defunkt:feature-foo' => 1,
+ 'defunkt:feature-bar' => 2,
+ }))
+ stub_request(:get, "https://api.github.com/repos/defunkt/hub/pulls?page=2&state=open").
+ to_return(:body =>
+ mock_list_pulls_response({
+ 'jianlius:feature-baz' => 3,
+ 'jianlius:feature-qux' => 4,
+ }))
+
+ expected = "open https://github.com/defunkt/hub/pull/2"
+ assert_command "browse -b feature-bar", expected
+
+ stub_branch('refs/heads/feature-qux')
+ stub_tracking('feature-qux', 'upstream')
+
+ expected = "open https://github.com/defunkt/hub/pull/4"
+ assert_command "browse -b", expected
+ end
+
def test_hub_browse_ssh_alias
with_ssh_config "Host gh\n User git\n HostName github.com" do
stub_repo_url "gh:singingwolfboy/sekrit.git"
@@ -525,6 +549,19 @@ def mock_pull_response(label, priv = false)
}
end
+ def mock_list_pulls_response(branches, name_with_owner = 'defunkt/hub', host = 'github.com')
+ arr = []
+ branches.each_pair do |branch, id|
+ arr << {
+ 'head' => {
+ 'label' => branch,
+ },
+ 'html_url' => "https://#{host}/#{name_with_owner}/pull/#{id}",
+ }
+ end
+ Hub::JSON.generate arr
+ end
+
def improved_help_text
Hub::Commands.send :improved_help_text
end
Something went wrong with that request. Please try again.