Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Merge pull request just by number #186

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

Carreau commented May 25, 2012

Hi, I'd like to be able to merge PR only by giving their number,
I was wondering if it is in hub philosophy.

I never wrote ruby before, but I came up with this... nor sure if it is the right way...

Also do you think it could be easy to get a hub prlist option that give an output of type :

pr number -- mergeable y/n  -- PR title 
pr number -- mergeable y/n  -- PR title 
pr number -- mergeable y/n  -- PR title 
pr number -- mergeable y/n  -- PR title 
...

thanks.
--
Matthias

Member

mislav commented May 25, 2012

Hey! It's a good idea. I definitely think there should be a way to specify only numbers instead of having to paste the whole URL.

Your code is good for start, but there are 2 smells with it. First, you accept an argument in form of "#xyz", but you are forgetting that "#" is a comment character in shell. So, to actually enter it in shell you would have to escape it:

git merge \#12

Don't worry. I fell down the same hole: 95a6dd5.

The second smell is that you're using the number to construct a whole URL, but in the next few lines of old code, it parses the URL to extract the pullreq number from it. So, you made it do the number->URL->number dance, which is unnecessary.

But this is all very good for your first attempts with Ruby.

I need to think about a better way to pass numbers to the command instead of the "#xyw" form.

As for your "prlist" wish, this is outside of scope of hub. But you can code something like that yourself. Here is a sample script that lists your open issues.

Carreau commented May 25, 2012

Your code is good for start, but there are 2 smells with it. First, you accept an argument in form of "#xyz", but you are forgetting that "#" is a comment character in shell.

Ah, true, except in zsh, where it works...

For number->URL->number dance I know, but I'm not confortable with ruby enough and hub internal to get further in short time.

As for the prlist, I have one but in python.

Thanks for the links, i'll maybe try to come up with something better when I'll be less busy and I had time to also learn ruby.

Carreau commented Jul 24, 2012

Ok, second try.
accept pull####,pr####,PR####,PR-####,pull/####,... as acceptable parameters.
I guess we could also match on number only...
all the changes after l374 are dedent.

@Carreau Carreau commented on the diff Jul 24, 2012

lib/hub/commands.rb
# > git fetch git://github.com/mislav/hub.git +refs/heads/feature:refs/remotes/mislav/feature
# > git merge mislav/feature --no-ff -m 'Merge pull request #73 from mislav/feature...'
def merge(args)
_, url_arg = args.words
- if url = resolve_github_url(url_arg) and url.project_path =~ /^pull\/(\d+)/
+ pull_id = nil
+ project_url = nil
+ if url_arg =~ /(pr|pull)(-|\/)?(\d+)/i
@Carreau

Carreau Jul 24, 2012

we could also match on number only by adding ? here : /(pr|pull)?(-|\/)?(\d+)/i

@Carreau Carreau commented on the diff Jul 24, 2012

lib/hub/commands.rb
- user, branch = pull_data['head']['label'].split(':', 2)
- abort "Error: #{user}'s fork is not available anymore" unless pull_data['head']['repo']
@Carreau

Carreau Jul 24, 2012

all the rest of the diff is only dedent, and remove of the final end.

Member

mislav commented Jul 25, 2012

This is getting somewhere, but I'm not linking the user API: "PR### pr### pull### pull/###"… Although it's easy to implement with regexp, supporthing this many different syntaxes is confusing. So far I'm leaning towards just having the "pull/###" syntax, because this pattern also occurs in web URLs. But I'm still not completely sold. Ideally it would be a command-line flag to git checkout/merge such as --pull=42

Carreau commented Jul 25, 2012

Would matching the flag with a regexp (/\-\-(pull|pr)=(\d+)) be accepted ?

Member

mislav commented Jul 26, 2012

I'm still not sure whether I would add this flag to checkout & merge commands, or make separate commands such as "checkout-pr" and "merge-pr". Lets put this on hold while I think about this

In the meantime you're free to code and use your own solution locally and let me know how it feels after some use. See defunkt#220 (comment)

Carreau commented Jul 26, 2012

No problem !

Carreau commented Jul 26, 2012

and thanks for the link !

Carreau commented Aug 22, 2012

@mislav
I think this gist might be of interest.
https://gist.github.com/3342247

greduan commented Oct 26, 2012

Bump for this, this is an awesome feature to have!

Carreau commented Oct 26, 2012

@greduan
With upper gist you can do git [merge | checkout | pull | ...] pr/#### it is not number only, but it works quite well.

greduan commented Oct 26, 2012

Oh that's cool. Still, would love to see it integrated as part of this ;)

ptn commented Dec 14, 2012

@mislav did you come to a decision about this? I started using hub seriously today and this was the first thing that popped into my mind as something that I'd like to use. What initially raised doubts?

I'm in favor of using git [merge|checkout] pull/NUM instead of adding a flag or creating a new command, since that'd be the least intrusive way to expand git.

greduan commented Dec 14, 2012

I agree with @ptn, that's a good format that really doesn't alter much, just gotta add the functionality.

Carreau commented Dec 14, 2012

I'm in favor of using git [merge|checkout] pull/NUM instead of adding a flag or creating a new command, since that'd be the least intrusive way to expand git.

Then you don't need to modify hub :

git config --global --add remote.origin.fetch "+refs/pull/*/head:refs/remotes/origin/pull/*"

ptn commented Dec 14, 2012

@Carreau I see. Didn't know that, thanks! Is this pull request still valid then?

Carreau commented Dec 14, 2012

Depends on wether @mislav still think this functionnality should be added by default or not ( I guess this is more a "request for ehancement" then) it is still nice to have tools that are able to do lots of stuff without having to configure each of them yourself.

ptn commented Dec 14, 2012

What if we turned this into something like hub config --fetch-pull-requests? That would add the configuration you proposed above. It'd be more of a shortcut, a convenience. I'm not sure though, since, as far as I'm aware, hub has never done anything with git configuration, so this would need a lot more work.

Carreau commented Dec 5, 2013

I don't think there any need to leave this open, I'll close; Thanks.

@Carreau Carreau closed this Dec 5, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment