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

Use repository API for merges when possible #585

Closed
wants to merge 3 commits into from
Closed

Use repository API for merges when possible #585

wants to merge 3 commits into from

Conversation

sstok
Copy link
Contributor

@sstok sstok commented Jul 30, 2016

Q A
Fixed tickets #584
License MIT

Work in progress, needs testing

@sstok
Copy link
Contributor Author

sstok commented Jul 30, 2016

OK, it works but it includes the Merge pull request #nn from sstok/xxxx header.
So that's something that needs work.

@cordoval
Copy link
Member

cordoval commented Jul 30, 2016

that does not need work, that is just ok, if user decided to merge it via the cloud is what it is supposed to say. It is ok right?

This of course does not meant that this PR has been reviewed just in case.

But it is starting to look better

@sstok
Copy link
Contributor Author

sstok commented Jul 30, 2016

It is fixable using the polaris-preview https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button

You can merge this as-is now, so that I can fix the merge message tomorrow 👍 (need to figure out how to pass extra headers to the client).

@sstok
Copy link
Contributor Author

sstok commented Jul 30, 2016

polaris-preview is not going to work, the Knp GitHub library doesn't support the commit_title yet and the method (put()) to do this manually is protected. Instead I added an option to allow disabling the merge-api.

@cordoval
Copy link
Member

cordoval commented Aug 1, 2016

@sstok please rebase PR

return false;
}

if ($input->getOption('squash') ||
Copy link
Contributor

Choose a reason for hiding this comment

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

You can still squash commits using the preview github API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but that will complicate the Adapter API 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Not too much though. Probably worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions? You need to detect whether the Adapter support this operation.
But I don't like adding yet another interface like CanSquashPullRequestOnMerge 💀

Adding an extra arguments on the method is no problem btw 👍

This speeds-up the merge process but still ensures additional
options are possible.
@sstok sstok closed this Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants