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

Create pull request with quick pick #661

Merged
merged 14 commits into from Nov 7, 2018

Conversation

Projects
None yet
3 participants
@rebornix
Copy link
Member

rebornix commented Nov 5, 2018

This PR reflects the recording shared in #620 (comment). It derived from 534dd00 in @queerviolet 's pull request #554 and the code change is minimal

  • Detect local remotes and their default targets, basically that's the logic in #554
  • Users only choose target remote and branch
  • We show a progress bar while creating Pull Request
  • Switch to Review Mode immediately and open the description view for that PR. We already merged #643 so users can update the description of the pull request with ease.

#554 shows all the possibilities of creating pull request: quick pick (modal), webview and pullrequest.md, and it inroduces Preact and Redux, which simplies the webview rendering code and improves the testability significantly. For short, we love both the UX and engineering improvement in #554. Instead of going back and forth there, let's branch out and use this pr to bring in the first iteration workflow.

@queerviolet I'd love to hear your ideas on how we can bring remote/branch logic from https://github.com/Microsoft/vscode-pull-request-github/pull/554/files#diff-16d81de09f288c2fbde3485db2c8003f into this pr. We can discuss this on Monday's syncup.

@rebornix rebornix requested a review from queerviolet Nov 5, 2018

@rebornix

This comment has been minimized.

Copy link
Member

rebornix commented Nov 5, 2018

@queerviolet if you see anything missing from #554, feel free to push directly to this branch or leave comments, both ways work.

* @param {string?} localBranch name of local branch to push (default: current branch)
* @param {string?} remoteBranch name of remote branch (default: name of local branch)
*/
export async function push(

This comment has been minimized.

@RMacfarlane

RMacfarlane Nov 5, 2018

Contributor

we should be able to use the git extension API for this, it was added recently. would require bumping engine to 1.29

This comment has been minimized.

@rebornix

rebornix Nov 5, 2018

Member

yes we should remove this if are are going to do push when creating the PR.

This comment has been minimized.

@RMacfarlane

RMacfarlane Nov 6, 2018

Contributor

we can remove this file, not used anywhere

try {
const { octokit, remote } = await this.ensure();
const { data } = await octokit.repos.get({
async getMetadata(): Promise<any> {

This comment has been minimized.

@RMacfarlane

RMacfarlane Nov 5, 2018

Contributor

in master there's a change to bump the version of the octokit/rest library, which adds return type information, so it should be easy to create an interface that extends that type to have currentUser

This comment has been minimized.

@rebornix

rebornix Nov 5, 2018

Member

agree and ideally we can have currentUser in @octokit/rest typings as well.

Show resolved Hide resolved src/view/reviewManager.ts Outdated
Show resolved Hide resolved src/view/reviewManager.ts Outdated
Show resolved Hide resolved src/view/reviewManager.ts Outdated
@@ -1025,6 +1028,55 @@ export class ReviewManager implements vscode.DecorationProvider {
await this.validateState();
}

public async createPullRequest(): Promise<void> {
const branchName = this._repository.state.HEAD.name;

This comment has been minimized.

@RMacfarlane

RMacfarlane Nov 5, 2018

Contributor

right now if you try to create a PR from local branch that hasn't been pushed to the remote, a notification with just "Validation Failed" is shown. should we push the branch in that case?

I think there may also be a problem when the local branch is named differently than the remote branch

This comment has been minimized.

@rebornix

rebornix Nov 6, 2018

Member

for the first iteration, we can show a warning for that case. We didn't decide yet how we are going to handle the push workflow.

Show resolved Hide resolved src/view/reviewManager.ts Outdated
Show resolved Hide resolved src/view/reviewManager.ts Outdated

rebornix added some commits Nov 6, 2018

@rebornix

This comment has been minimized.

Copy link
Member

rebornix commented Nov 6, 2018

Addressed comments except #661 (comment) (we can update this after we discuss how to handle push like in #554) and #661 (comment) (which can probably be updated in upstream).

rebornix added some commits Nov 6, 2018

@RMacfarlane
Copy link
Contributor

RMacfarlane left a comment

LGTM!

@rebornix rebornix merged commit 14a37f0 into master Nov 7, 2018

1 check passed

license/cla All CLA requirements met.
Details

@rebornix rebornix deleted the rebornix/create-pr-quickpick branch Nov 20, 2018

@Ghostbird Ghostbird referenced this pull request Jan 22, 2019

Open

Discussing Create PR #620

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