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

A pull request to create pull requests #554

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@queerviolet
Copy link
Collaborator

queerviolet commented Oct 5, 2018

This PR adds a pull request creation flow.

  • New webview type: NewPR. We add a new type of webview, defined in newPRPanel.ts. I'd love to eliminate this and switch it out for a Markdown buffer with special Intellisense, but I think there are some rough edges on that interaction: how do you submit? Does closing the panel do it? Is that expected, or odd? So perhaps that's a future improvement.
  • We use preact in the nested page
  • The webview communicates with the extension via a Redux store. I'm not a Redux partisan by any means, but it is pretty nice for this use case, where we want to synchronize serializable state and mutations to that state across two VMs.
@rebornix

This comment has been minimized.

Copy link
Member

rebornix commented Oct 8, 2018

I played with current code a bit and it works pretty well with typing in title/desc/target-branch ;)

What I'm not sure about is what's the expected first step for the pull request creation workflow in an editor: providing title/desc or reviewing the file changes first. The latter is the one I want to do first. We can probably create a tree view, similar to "Changes in Pull Request" and it contains file changes and commits. The description node for this tree view is now a pull request submit form where we type in necessary information for the pull request creation. Or we can use Quick Pick UI to do the submition work, like what this PR already implements. @queerviolet what do you think?

@queerviolet

This comment has been minimized.

Copy link
Collaborator

queerviolet commented Oct 9, 2018

I think what we want to do long term is to immediately create a draft PR. Draft PRs are not pushed to the server—at least not publicly—but they otherwise have the same view as other PRs. We could either create another top-level section in the Github Pull Requests sidebar tree view (Drafts), or we could put them under Created By Me with badges that say Draft.

This way, you don't have to learn another interface, and we don't introduce another interface mode. You can edit the draft PR exaclty how you can edit any other PR you create, at a leisurely pace, and once you're ready, you can click a Publish PR button that publishes the PR.

I'm working on some mocks of this flow right now that we can discuss. Generally, it will require the following changes:

  1. Enabling editing on the description page (which we want to do anyway)
  2. Either finding somewhere to save draft PR data locally metadata locally, or adding backend support for draft PRs.

Since it's a bit more involved, I wanted to get this feature rolling first with absolutely minimal UX. The evolution I see is:

  1. This PR: Create PR with super minimal UI
  2. Soon: Editing PRs
  3. Draft PRs?

1 & 2 get us 90% of the way to where we need to be. With those two features in, you'll be able to create a [WIP] PR and edit it on the description page. The only thing you won't be able to do is preview a PR before sending it to the server. I think being able to do that is important in terms of building confidence—i.e., overcoming fear of submitting a PR that isn't quite perfect. But it doesn't really enable anything that you couldn't do before, provided you're willing to edit in public a bit.

So, in short: I agree, but think that should be a (near) future evolution of this feature.

@rebornix rebornix referenced this pull request Oct 10, 2018

Closed

Push in Git's API #60547

@queerviolet

This comment has been minimized.

Copy link
Collaborator

queerviolet commented Oct 10, 2018

So I've changed my mind. I don't think it's worth it to try for a version of this with a super-minimal UI. Common cases have too much state to work with before submitting, and asking a series of questions with the prompt UI is scary and unwieldy.

Consider a common case:

  1. I clone an upstream repo, which I do not have permission to write to. (Perhaps I should have forked, but I wasn't thinking about it).
  2. I do some work in a local branch in that repo.
  3. I invoke create PR.

Now what?

I think, ideally, we'll create a fork of the upstream and push our local branch there, and then create a branch against the fork. But that's a lot of magic to just "have happen". Ideally, I'd want the user to at least see what's going to happen on the PR description page before clicking Create PR, which means going ahead and creating that interface.

I'll post some of the mocks we used for UX testing, so we can discuss them. Also, I'm planning on saving draft PR information in git config settings, since we're already saving information there, and those can, it appears, hold arbitrary amounts of data.

@sguthals

This comment has been minimized.

Copy link
Collaborator

sguthals commented Oct 10, 2018

You're right about this common workflow @queerviolet.

As of today, the way we handle PRs when you don't have push access is to have users create a Fork. This is a more complex flow that we should definitely open up another issue for it and have that be the next thing we tackle in this flow.

The basic flow for creating a PR is as such:

  1. You’re currently on a branch
  2. You have push access to upstream
  3. You choose a target branch from the repo
  4. You add a title
  5. You add a description
  6. You click “Create PR” which will immediately create the PR on dotcom and now you have all the PR functionalities in VS Code
    And for right now:
  7. If you don’t have push access, we fail gracefully

Do you want to open an issue to handle 7 in the above list better?

@queerviolet

This comment has been minimized.

Copy link
Collaborator

queerviolet commented Oct 10, 2018

Let's just see how this works first.

@queerviolet queerviolet force-pushed the f/qv-create-pr branch 2 times, most recently from 9243b00 to 3d3d8a9 Oct 12, 2018

@Astrantia

This comment has been minimized.

Copy link

Astrantia commented Oct 18, 2018

@queerviolet The build failed on my Windows machine. Can you post pictures for a demo? 😅

@queerviolet queerviolet force-pushed the f/qv-create-pr branch from 806bb11 to 8c29867 Oct 23, 2018

@queerviolet queerviolet changed the title [WIP] A pull request to create pull requests A pull request to create pull requests Oct 23, 2018

@queerviolet queerviolet requested review from RMacfarlane and rebornix Oct 23, 2018

@queerviolet

This comment has been minimized.

Copy link
Collaborator

queerviolet commented Oct 23, 2018

Okay, this is ready for review.

I'll probably want to add some tests in a bit, but this has become a bit of a monster, so let's get the ball rolling.

@@ -0,0 +1,57 @@
import { Upstream } from './state';

This comment has been minimized.

@RMacfarlane

RMacfarlane Oct 24, 2018

Contributor

would it make sense to move the new shared and store folders into preview-src since they're being used by the webview?

This comment has been minimized.

@RMacfarlane

RMacfarlane Oct 24, 2018

Contributor

Nevermind, I see that we import from store on the extension side

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

This comment has been minimized.

@RMacfarlane

RMacfarlane Oct 24, 2018

Contributor

can we add a type for this metadata?

import { Disposable, Event } from 'vscode';
import { init } from './actions';

export class Store {

This comment has been minimized.

@RMacfarlane

RMacfarlane Oct 25, 2018

Contributor

is this meant to ultimately store all of the extension's state? what's the responsibility of this class?

Centralizing state is fine, but I'm concerned that we now have duplicate sources of truth for some of the state - ex. the PullRequestManager is also tracking githubRepositories

@@ -34,6 +36,39 @@ interface RestError {
field: string;
resource: string;
}
export class NoGitHubReposError extends Error {

This comment has been minimized.

@RMacfarlane

RMacfarlane Oct 25, 2018

Contributor

should we extract errors to their own file?

@kieferrm

This comment has been minimized.

Copy link
Contributor

kieferrm commented Oct 25, 2018

Pls see #620.

@kieferrm kieferrm referenced this pull request Oct 29, 2018

Closed

Plan for October 2018 #544

9 of 18 tasks complete
@sguthals

This comment has been minimized.

Copy link
Collaborator

sguthals commented Nov 8, 2018

@queerviolet Should we close this one?

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