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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create pull request #1376

Merged
merged 73 commits into from Apr 12, 2018

Conversation

Projects
None yet
5 participants
@smashwilson
Member

smashwilson commented Apr 3, 2018

Implement the "create pull request" design from #1138. Restarting this in a new pull request because most of the existing work ended up going in to #1230, although I intend to heavily reference the code there 馃槈

Here's @simurai's design:

Fixes #911.

  • Disable the button and show progress while pushing
  • Change messaging on the repository's main branch
  • Special messaging when you're on a non-main ref, but haven't committed anything yet
  • Support weird push refspecs when determining which local branch corresponds to the remote's default branch
  • Custom messaging when you have a detached HEAD.
  • Handle the case when the current branch is configured to push to a different remote.
@@ -11,16 +11,29 @@ export class PrSelectionByBranch extends React.Component {
static propTypes = {
repository: PropTypes.shape({
pullRequests: PropTypes.shape({
totalCount: PropTypes.number,
totalCount: PropTypes.number.isRequired,
edges: PropTypes.arrayOf(PropTypes.shape({

This comment has been minimized.

@annthurium

annthurium Apr 3, 2018

Contributor

yay, thank you for using PropTypes.shape!

@simurai

This comment has been minimized.

Member

simurai commented Apr 4, 2018

@smashwilson Not sure how far along you already are?

Were you thinking of having the "Push" button too? In case commits aren't pushed yet? With #1308 being merged, it probably should say "Publish". Or not have the button and just add a hint:

In order to create a Pull Request, "Publish" first your changes from the Status Bar.

@50Wliu

This comment has been minimized.

Member

50Wliu commented Apr 4, 2018

Would it be too confusing to always just have a single "Create Pull Request" button that publishes and pushes the branch if necessary?

@simurai

This comment has been minimized.

Member

simurai commented Apr 4, 2018

a single "Create Pull Request" button

Yeah, I think that would be even better. 馃憤 Is the "Pushing" status-bar tile enough of a hint? Otherwise the button could have an "in progress" state before opening the browser to .com's compare page. Like [Create Pull Request] -> [Publishing...] (disabled button) -> open browser. So that people don't wonder why nothing happens after clicking.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Apr 4, 2018

@smashwilson Not sure how far along you already are?

Not super far, and I'm working to prep a demo today, but I also don't think it'll take that long. I mostly need to get props passed down through the component hierarchy at this point, then take it for a spin and give it some "developer art" CSS until @simurai has bandwidth to make it look good. Hopefully I'll have something usable by the end of the week? 馃槃

a single "Create Pull Request" button

Ooh, I like that. Simplicity++

The only downside I can see is that you have no way to not push local commits in the (rare) case when you don't want to - if the branch is published, but you've done some additional work locally that you explicitly don't want to push just yet, but you want to get the PR opened. Sounds uncommon and you can always just go to the browser instead.

It might be nice to change the messaging on the button slightly, to make it clear that we're going to publish a branch or push commits for you? Just so we don't catch anyone by surprise by making something public.

@simurai

This comment has been minimized.

Member

simurai commented Apr 5, 2018

The only downside I can see

Always finding those edge cases. 馃榿

It might be nice to change the messaging

  • [ Publish + Open New Pull Request ]
  • [ Push + Open New Pull Request ]

Maybe "Publish..." since it says the same in the status bar?

smashwilson and others added some commits Apr 3, 2018

Add space after link
Otherwise it says Search againto
@smashwilson

This comment has been minimized.

Member

smashwilson commented Apr 10, 2018

Here's the messaging that I've settled on for the moment. Blockquote text appears just below the "new pull request" icon; text [in brackets] is the button text, if present.


When you're on a detached HEAD:

You are not currently on any branch. Create a branch to share your work with a pull request.

When you're on a branch that pushes to your repository's default branch:

You are currently on your repository's default branch. Create a new branch to share your work with a pull request.

When you're on a ref that has the same SHA as your repository's default branch:

Your current branch has not moved from the repository's default branch. Make some commits to share your work with a pull request.

If your current branch is currently configured to push to a different remote:

Your current branch is configured to push to the remote upstream. Publish it to origin instead?

[Publish + open new pull request]

If your current branch has no upstream:

[Publish + open new pull request]

If your current branch has an upstream, but has unpushed commits:

[Push + open new pull request]

If your current branch has an upstream and it's up to date:

[Open new pull request]


I'd 鉂わ笍 any thoughts and feedback about the wording 馃槃

@smashwilson

This comment has been minimized.

Member

smashwilson commented Apr 10, 2018

There were a few tradeoffs in how I've implemented this, too. In particular, we won't detect:

  • We won't find the default branch if you don't have a local branch that tracks it. This is because I'm finding it from the output of git for-each-ref refs/heads/**. I could widen that to refs/**, but I don't want to overwhelm us in repos with many remote references.
  • If your local branch that tracks the repository's default branch moves ahead after you create an empty branch. .com will still show you a "nothing to compare" view here, but I can't detect this easily without finding ahead/behind statistics on every branch.

@smashwilson smashwilson requested review from annthurium and kuychaco Apr 10, 2018

@@ -6,23 +6,23 @@ import yubikiri from 'yubikiri';
import RemotePrController from './remote-pr-controller';
import GithubLoginModel from '../models/github-login-model';
import ObserveModel from '../views/observe-model';
import {RemotePropType} from '../prop-types';
import {RemotePropType, BranchPropType} from '../prop-types';

This comment has been minimized.

@annthurium

annthurium Apr 10, 2018

Contributor

question: do we have any conventions around import ordering or is it YOLO / free for all?

This comment has been minimized.

@smashwilson

smashwilson Apr 11, 2018

Member

100%

yolo

I'm assuming eslint can enforce an ordering for us if we want? It hasn't really been an issue for us yet, but I'd be fine with turning that on in the name of tidiness 馃槃

This comment has been minimized.

@annthurium

annthurium Apr 11, 2018

Contributor

ehh, I find eslint's ordering import rule to be really weird and arbitrary and hard to remember.
https://eslint.org/docs/rules/sort-imports

I'm ok with yolo tbh! Or with a like "try to alphabetize things but nbd if you don't"

border-top: 1px solid @base-border-color;
}
&-cell {

This comment has been minimized.

@annthurium

annthurium Apr 10, 2018

Contributor

<3 flexbox

bar = new Branch('bar', Branch.createRemoteTracking('upstream/bar', 'upstream', 'refs/heads/bar'));
bs.add(bar);
// A branch with an upstream and push that use some weird-ass refspec

This comment has been minimized.

@annthurium

annthurium Apr 10, 2018

Contributor

thanks for all the comments! Super helpful.

@kuychaco

Got about halfway through my review. All looks good so far! Just left a few questions for ya @smashwilson. Will pick up tomorrow and review the rest :)

});
}
async getRefSha(ref) {

This comment has been minimized.

@kuychaco

kuychaco Apr 11, 2018

Member

Did a quick search for getRefSha and can't seem to find where it is used...?

This comment has been minimized.

@smashwilson

smashwilson Apr 11, 2018

Member

Ahhhh right. I added that anticipating that I'd need it to see if HEAD had moved from the default branch, but it turns out to be a lot easier to get that from git for-each-ref instead.

Cleaned up, good catch 馃憤

getPush() {
return this;
},

This comment has been minimized.

@kuychaco

kuychaco Apr 11, 2018

Member

Do we also want isHead and getSha methods on here?

This comment has been minimized.

@smashwilson

smashwilson Apr 12, 2018

Member

... Yes, yes we absolutely do 馃槅

Good catch 馃檱

getRemoteName() {
if (!this.isRemoteTracking()) {
return Symbol('not-remote-tracking');

This comment has been minimized.

@kuychaco

kuychaco Apr 11, 2018

Member

This is mostly to satisfy my own curiosity - we have constants for detached and remote-tracking symbols at the top of this file. Which I understand is to make these properties private to this class.

Looks like you're also using symbols as return values for null/non-existent values. Could you elaborate on the thought behind it? And is there any reason that we don't store these symbols in constants (for not-remote-tracking, no-sha, null-branch-ref, null-branch-remote-name, and null-branch-remote-ref)? Do we want them all to be unique and be referentially non-identical? If we don't care about this, is it worth it to use a single constant and save on symbol creation?

This comment has been minimized.

@smashwilson

smashwilson Apr 11, 2018

Member

Do we want them all to be unique and be referentially non-identical?

This is exactly what I was going for. I wanted the result of calling .getRemoteName() and the other conditional getters on a Branch to which they don't apply to never compare === to the result from any other Branch, valid or invalid. If I returned '' or a shared Symbol, this could happen:

// Detached branch
const b0 = Branch.detached('HEAD~2');

// Branch with no upstream
const b1 = new Branch('untracked');

if (b0.getRemoteName() === b1.getRemoteName()) {
  console.log('oops');
}

Mostly, I wanted to avoid needing to do a bunch of if guards any time I wanted to compare Branches in PrSelectionByBranch - in general, callers would need to know which of isPresent(), isDetached(), isRemoteTracking() to check on the operands before you checked the results of any given getter.

The downside of doing this - in addition to looking a little weird - is that now you can no longer display getRemoteName() nicely in UI without having to do an isRemoteTracking() check first. It's also inconsistent with the way other null object getters behave (which tend to return something like '' for uniform display). I'd convinced myself yesterday that this was worth it to streamline the comparisons I was doing, but now I'm not so sure. What do you think?

This comment has been minimized.

@kuychaco

kuychaco Apr 11, 2018

Member

Ah yes, makes sense. I think either way - no super strong feelings, so I leave it up to you 馃槃.

I guess one guiding question could be: which do you expect to be more likely - comparing branches or displaying branch names in the UI? I did a quick check throughout the codebase and didn't seem to find any branch comparisons, but perhaps I just missed them.

But in the end, either way is fine and I'm cool to leave as-is 馃憤

This comment has been minimized.

@smashwilson

smashwilson Apr 12, 2018

Member

I did a quick check throughout the codebase and didn't seem to find any branch comparisons, but perhaps I just missed them.

... Nope, that's because there aren't any any more! I think this is another case where I'd designed a bit of the model layer with one revision of the container code in mind, and ended up implementing the container in a different way.

I guess one guiding question could be: which do you expect to be more likely - comparing branches or displaying branch names in the UI?

With that in mind, this answer definitely becomes "displaying branch names in the UI." Modified accordingly 馃憤

smashwilson added some commits Apr 11, 2018

GraphQL returns a prefix and name for refs
I don't _think_ it can ever be something but refs/heads. But, hey.
Don't die horribly when there is no defaultRef
Hat tip to @kuychaco for finding this edge case!
@smashwilson

This comment has been minimized.

Member

smashwilson commented Apr 11, 2018

Two more edge cases handled.

If the repository has just been created on .com and has no default ref, we display:

screen shot 2018-04-11 at 10 49 02 am

If the repository no longer exists, we display:

screen shot 2018-04-11 at 11 05 22 am

@kuychaco

This is great @smashwilson! Nice job catching/handling the many edge cases :shipit: 馃挜

@kuychaco

This comment has been minimized.

Member

kuychaco commented Apr 12, 2018

One last question...

We won't find the default branch if you don't have a local branch that tracks it. This is because I'm finding it from the output of git for-each-ref refs/heads/. I could widen that to refs/, but I don't want to overwhelm us in repos with many remote references.

@smashwilson could you elaborate on the edge case where this might be problematic? Thanks!

If your local branch that tracks the repository's default branch moves ahead after you create an empty branch. .com will still show you a "nothing to compare" view here, but I can't detect this easily without finding ahead/behind statistics on every branch.

馃憤 this seems like a good tradeoff to make

smashwilson added some commits Apr 12, 2018

@smashwilson

This comment has been minimized.

Member

smashwilson commented Apr 12, 2018

could you elaborate on the edge case where this might be problematic?

Sure - it's a bit of a stretch 馃槃

To identify the situation of being on a non-default branch that's still identical to the default, I'm:

  1. Finding the default branch name and prefix from the GraphQL API.
  2. Collecting results from git for-each-ref --format=... refs/heads/**, which includes some helpful data in each row, notably the current sha and the remote name and remote ref (taking into account all of the various and sundry ways you can specify a refspec) for both fetch and push destinations of each branch. The BranchSet indexes these in a few Maps so that we can do constant lookups instead of linear-time scans on every render.
  3. Finding the sha of any branches that push to the default ref we got from (1) by finding the row from (2) that lists the matching remote ref as a push destination.
  4. If any of those shas match the sha from HEAD, I conclude that we haven't made any commits yet and show that prompt.

In the (pretty uncommon?) case where you've just done a clone of a repo and checked out a non-default branch:

git clone --branch aw/create-pr git@github.com:atom/github.git

In this case, our default branch is refs/heads/master, but your clone doesn't have any refs under refs/heads/** that push there, so we won't be able to find any sha in (3), so we won't be able to tell if HEAD has moved or not and we'll always show the "open a new PR" prompt.

We could avoid this by changing the for-each-ref call to something like git for-each-ref --format=... refs/** to include remote tracking branches and tags and everything in the output we collect for (2), but it's a git command that we execute pretty frequently, and I was hesitant to dramatically increase the output we process from it when you have a repo with a lot of tags and remote tracking branches, especially because (a) it doesn't cache really well, (b) the edge case it would fix is super uncommon, and (c) the behavior when we miss it isn't that bad.

This super-long explanation brought to you by the magic of 鈽曪笍

@smashwilson smashwilson changed the title from [wip] Create pull request to Create pull request Apr 12, 2018

@smashwilson smashwilson merged commit 6c0f926 into master Apr 12, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Short-Term Roadmap automation moved this from In Progress 馃捇 to Complete 鉁 Apr 12, 2018

@smashwilson smashwilson deleted the aw/create-pr branch Apr 12, 2018

@smashwilson smashwilson removed this from Complete 鉁 in Short-Term Roadmap Apr 23, 2018

@smashwilson smashwilson referenced this pull request Jun 21, 2018

Merged

Pull Request list #1523

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