You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thanks @safv12, thanks for the issue! This is definitely something we're potentially open to down the road but we want to make sure we're delivering the right context to people so they have sufficient information to be able to confidently merge. So we want to be really thoughtful about its implementation. I'm going to leave this open as something we're open to and add that it needs design thinking before we proceed. Thanks again!
Reacted by Francisco Ramón Sánchez Favela, Borys Hulii and Aliabbas Merchant
Thanks @billygriffin! I appreciate that you're taking the time and thought to make the PR merge experience very polished.
I really like what I've see so far, but currently it doesn't offer the advertised "Goodbye, context switching" value proposition if following Github flow still requires opening a browser to merge the PR.
That's not intended as a complaint, this is great work so far and I realize you want to solicit feedback on the current progress at this stage. As part of that feedback I just wanted to add my 2c that this is a very key feature I think fits as high priority on the roadmap. 😀
We discussed this a bit more today, and I just wanted to call out a few considerations prior to implementation:
We've drawn lots of inspiration here from the conversation in mislav/hub#2280 (so thanks to all who weighed in there), and we're thinking about this command as:
gh pr merge [pr number or URL] with optional flags --merge, --squash, --rebase as options assuming those are possible via the API.
To account for the possibility that people likely want a bit more control over which type of merge is done, we think v1 of this could likely have an interactive component to allow people to choose which merge method to use.
The tradeoff here is that we're requiring people who just want to use a normal merge and don't care about rebasing or squashing to either select an option interactively or use a flag, and there's no default on the command that just does the merge. However, given the importance of understanding what's about to happen from a user's perspective, we think that tradeoff may be reasonable for a first iteration.
I think that tradeoff makes sense to me! It also aligns with the intuitive behaviour already established from gh pr create in my opinion where, without any flags, you will be dropped into an interactive mode to do the operation you want, but it still has the flexibility to function transactionally by providing full flags removing the ambiguity.
You can still make a normal merge the "default" by having it be the first option. I'm not sure how you intend to implement the interactivity but if it's an option list of three choices you move between with arrow keys then the first one could just be the default merge (or possibly even similar to the web UI where I think it remembers your last choice) so that users only have to press Enter. If it's a typed input then you could also default to Enter (with empty inputs) doing the normal merge operation. What do you think?
@billygriffin@mislav having this feature on is a massive step fwd especially for hub users like me who needs to do s'thing like
hub api -XPUT repos/{owner}/{repo}/pulls/6/merge -F merge_method=squash -F commit_title="$(git log -n 1 --pretty=format:'%B' <commit sha> | sed -n 1p)" -F commit_message="$(git log -n 1 --pretty=format:'%B' <commit sha>| sed '1d; /./,$!d')"
Note i'm controlling the commit title and message as that is the key to avoid the garbage added by the UI
the reason i need to do so is because if you have a PR with n commits and lots of fixups when you do squash and merge the UI adds a lot of info which change the commit title & message and what you end up after the merge is a commit with a bad title/ description.
And sadly there is no way you can prevent that using GHA.
Please consider and bump the priority of this feature, thanks !
@DanyC97 Thanks for sharing! This is already pretty high priority and in the next set of things we're working on, so I'm glad to hear it will be useful for you!
Reacted by Dani Comnea, Nour Haridy and Aliabbas Merchant
I still want to add my concern about gh pr merge having any defaults!
Please see my initial comment here about I would envision the workflow: mislav/hub#2280 (comment)
Reading the comments I can see my concern was well heard, so thanks for listening 🙉
Reacted by Amanda Pinsker, Billy Griffin, Corey Johnson and Sandro
Is there any chance you guys could consider a custom logic for merging?
That's certainly an interesting proposal, but us folks will likely focus on just supporting the equivalent of the Merge Button from gh pr merge. I think that users are already able to script their more convoluted git workflows on their own, i.e. outside of gh.
Is there any chance you guys could consider a custom logic for merging?
That's certainly an interesting proposal, but us folks will likely focus on just supporting the equivalent of the Merge Button from gh pr merge. I think that users are already able to script their more convoluted git workflows on their own, i.e. outside of gh.
@mislav i hear and understand your angle (since i also read your blog between hub vs gh) however (with all respect) i have to disagree with you here.
Having all 3 options shown in the GH UI: Merge, Squash and Merge and Rebase and merge i don't think/ it shouldn't be outside of gh. At the end of the day is not a custom workflow, is just what the UI supports ported to the cli.
I agree and support the vision of not turning gh into hub however having a gh cli supporting what the GH API and the UI provides should (dare to say must) be in the scope, please reconsider it.
@DanyC97 I don't think @mislav was suggesting we don't include support for the three primary options, and in fact the PR for this is in progress and does include all three (merge, squash and merge, rebase and merge). The response was to supporting more custom workflows beyond those with, for example, a --custom flag.
@DanyC97 I don't think @mislav was suggesting we don't include support for the three primary options, and in fact the PR for this is in progress and does include all three (merge, squash and merge, rebase and merge). The response was to supporting more custom workflows beyond those with, for example, a --custom flag.
as i see @billygriffin , please accept my aplogoise for misunderstanding, thank you!
For example, if a PR is merged into branch develop, I want to sync my local clone of the repo so that my local develop is also up to date with the merged changes.
Reacted by Mislav Marohnić, Rohan Sathe and Jesse Glick
Activity
billygriffin commentedon Feb 12, 2020
Thanks @safv12, thanks for the issue! This is definitely something we're potentially open to down the road but we want to make sure we're delivering the right context to people so they have sufficient information to be able to confidently merge. So we want to be really thoughtful about its implementation. I'm going to leave this open as something we're open to and add that it needs design thinking before we proceed. Thanks again!
chrisfosterelli commentedon Feb 12, 2020
Thanks @billygriffin! I appreciate that you're taking the time and thought to make the PR merge experience very polished.
I really like what I've see so far, but currently it doesn't offer the advertised "Goodbye, context switching" value proposition if following Github flow still requires opening a browser to merge the PR.
That's not intended as a complaint, this is great work so far and I realize you want to solicit feedback on the current progress at this stage. As part of that feedback I just wanted to add my 2c that this is a very key feature I think fits as high priority on the roadmap. 😀
dav-is commentedon Feb 20, 2020
This is something I'd be interested in, especially if I'm able to sign squash merges
billygriffin commentedon Feb 28, 2020
We discussed this a bit more today, and I just wanted to call out a few considerations prior to implementation:
We've drawn lots of inspiration here from the conversation in mislav/hub#2280 (so thanks to all who weighed in there), and we're thinking about this command as:
gh pr merge [pr number or URL]with optional flags--merge,--squash,--rebaseas options assuming those are possible via the API.To account for the possibility that people likely want a bit more control over which type of merge is done, we think v1 of this could likely have an interactive component to allow people to choose which merge method to use.
The tradeoff here is that we're requiring people who just want to use a normal merge and don't care about rebasing or squashing to either select an option interactively or use a flag, and there's no default on the command that just does the merge. However, given the importance of understanding what's about to happen from a user's perspective, we think that tradeoff may be reasonable for a first iteration.
@ampinsk @mislav @vilmibm Do you have anything else to add?
OliverJAsh commentedon Mar 3, 2020
I wrote a script to do this with
hub, along with shell completions: https://gist.github.com/OliverJAsh/929c761c8ecbf14d0010634a3f015740#file-demo-pngIt would be really helpful if we could provide similar shell completions with the command proposed here.
chrisfosterelli commentedon Mar 5, 2020
I think that tradeoff makes sense to me! It also aligns with the intuitive behaviour already established from
gh pr createin my opinion where, without any flags, you will be dropped into an interactive mode to do the operation you want, but it still has the flexibility to function transactionally by providing full flags removing the ambiguity.chrisfosterelli commentedon Mar 5, 2020
You can still make a normal merge the "default" by having it be the first option. I'm not sure how you intend to implement the interactivity but if it's an option list of three choices you move between with arrow keys then the first one could just be the default merge (or possibly even similar to the web UI where I think it remembers your last choice) so that users only have to press Enter. If it's a typed input then you could also default to Enter (with empty inputs) doing the normal merge operation. What do you think?
ran-eh commentedon Apr 28, 2020
Also
--fill(analogous to pr create) and--forceto allow admins to override policy (require review etc.)DanyC97 commentedon Apr 29, 2020
@billygriffin @mislav having this feature on is a massive step fwd especially for
hubusers like me who needs to do s'thing likehub api -XPUT repos/{owner}/{repo}/pulls/6/merge -F merge_method=squash -F commit_title="$(git log -n 1 --pretty=format:'%B' <commit sha> | sed -n 1p)" -F commit_message="$(git log -n 1 --pretty=format:'%B' <commit sha>| sed '1d; /./,$!d')"Note i'm controlling the commit title and message as that is the key to avoid the garbage added by the UI
the reason i need to do so is because if you have a PR with n commits and lots of
fixupswhen you dosquash and mergethe UI adds a lot of info which change the commit title & message and what you end up after the merge is a commit with a bad title/ description.And sadly there is no way you can prevent that using GHA.
Please consider and bump the priority of this feature, thanks !
billygriffin commentedon Apr 29, 2020
@DanyC97 Thanks for sharing! This is already pretty high priority and in the next set of things we're working on, so I'm glad to hear it will be useful for you!
jetersen commentedon May 5, 2020
I still want to add my concern about
gh pr mergehaving any defaults!Please see my initial comment here about I would envision the workflow: mislav/hub#2280 (comment)
Reading the comments I can see my concern was well heard, so thanks for listening 🙉
fabriziocucci commentedon May 6, 2020
Is there any chance you guys could consider a custom logic for merging?
Something like:
where the custom logic could be a simple file in
<repo>/.gh/pr-merge-custom.This could be the most flexible way to support anything from simple flags (e.g.
--no-ff) to more convoluted workflows.mislav commentedon May 6, 2020
That's certainly an interesting proposal, but us folks will likely focus on just supporting the equivalent of the Merge Button from
gh pr merge. I think that users are already able to script their more convoluted git workflows on their own, i.e. outside ofgh.DanyC97 commentedon May 8, 2020
@mislav i hear and understand your angle (since i also read your blog between hub vs gh) however (with all respect) i have to disagree with you here.
Having all 3 options shown in the GH UI:
Merge,Squash and MergeandRebase and mergei don't think/ it shouldn't be outside ofgh. At the end of the day is not a custom workflow, is just what the UI supports ported to the cli.I agree and support the vision of not turning
ghintohubhowever having agh clisupporting what the GH API and the UI provides should (dare to say must) be in the scope, please reconsider it.billygriffin commentedon May 8, 2020
@DanyC97 I don't think @mislav was suggesting we don't include support for the three primary options, and in fact the PR for this is in progress and does include all three (merge, squash and merge, rebase and merge). The response was to supporting more custom workflows beyond those with, for example, a --custom flag.
DanyC97 commentedon May 10, 2020
as i see @billygriffin , please accept my aplogoise for misunderstanding, thank you!
OliverJAsh commentedon May 27, 2020
#899 looks great!
However there are a few missing parts which are preventing me from switching from my custom script:
pullchanges on local base branch (partially related to CLI command to clean up after a merged PR #380)jetersen commentedon May 27, 2020
@OliverJAsh those features does indeed look tempting.
Though as a initial version, this is a really good start.
Many thanks @probablycorey 👏
mislav commentedon May 27, 2020
@OliverJAsh Could you explain more what you meant with this?
OliverJAsh commentedon May 27, 2020
For example, if a PR is merged into branch
develop, I want to sync my local clone of the repo so that my localdevelopis also up to date with the merged changes.OliverJAsh commentedon May 27, 2020
https://gist.github.com/OliverJAsh/929c761c8ecbf14d0010634a3f015740#file-git-merge-pr-L49-L52
jglick commentedon May 27, 2020
A component of my suggestions in #380, though there was resistance to this from @eXamadeus.
models - ollama - async (cli#373)