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

Add git-checkout-pr feature #3596

Merged
merged 54 commits into from
Nov 3, 2020
Merged

Conversation

yakov116
Copy link
Member

@yakov116 yakov116 commented Sep 25, 2020

  1. LINKED ISSUES:
    Closes Can RG recommend me the command to fetch a PR locally? #2767

  2. TEST URLS:
    Remote URL: Mark peer dependencies as optional twbs/bootstrap#30329
    Upstream URL: karma: stop excluding polyfill.js from istanbul twbs/bootstrap#30740
    Own PR: undefined
    Local PR: I and I am collaborator, I removed the exclude for merged PR from it. You can probably find a PR since you are a collaborator in more repositories then me :).

  3. SCREENSHOT:
    image

source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
@yakov116

This comment has been minimized.

@yakov116

This comment has been minimized.

@yakov116 yakov116 marked this pull request as ready for review October 20, 2020 13:14
readme.md Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
@yakov116
Copy link
Member Author

Draft until after lint PR is merged

@yakov116 yakov116 marked this pull request as ready for review October 26, 2020 22:49
@yakov116
Copy link
Member Author

All working with isLocalPR dropped
I tested on this PR (My own)
webpack/webpack#11831
webpack/webpack#11824

I don't have a collaborator with a local PR to test on (but I manually tested on a merged and it worked)

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

I think the logic is fine now, but I haven’t tested all the cases. Can you test them and include your testing links? (Of course they’ll be related to you as the viewer)

source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
source/features/git-checkout-pr.tsx Outdated Show resolved Hide resolved
yakov116 and others added 2 commits October 27, 2020 08:25
Co-authored-by: Federico <me@fregante.com>
@yakov116
Copy link
Member Author

I think the logic is fine now, but I haven’t tested all the cases. Can you test them and include your testing links? (Of course they’ll be related to you as the viewer)

I tested them all. I keep posting links but they keep getting merged 😿 .

@fregante
Copy link
Member

fregante commented Oct 27, 2020

What I do is find old PRs on large, famous repos so there’s no chance that they get merged quickly.

@yakov116
Copy link
Member Author

What I do is find old PRs on large, famous repos so there’s no chance that they get merged quickly.

Like octoicon 😆 ?

@yakov116
Copy link
Member Author

Remote URL: twbs/bootstrap#30329
Upstream URL: twbs/bootstrap#30740
Own PR: undefined
Local PR: I and I am collaborator, I removed the exclude for merged PR from it. You can probably find a PR since you are a collaborator in more repositories then me :).

@yakov116
Copy link
Member Author

@fregante do you mind to review? It would be great to have this ship tomorrow!

@yakov116
Copy link
Member Author

yakov116 commented Nov 1, 2020

@fregante now is the perfect time to review this as we both have PR's open (from local branchs)

@fregante
Copy link
Member

fregante commented Nov 3, 2020

I'll extract the remote logic here because the review comment is easily lost:

It's kinda complicated and needs a little guessing, actually. Examples:

  • as a collaborator of this repo, origin points to this repo
  • you're also a collaborator of this repo, but I don't think that origin points to this repo yet since you still open

In short, if I see a PR opened from a branch here, I can pull origin, but if you see it, you will pull upstream, I'm guessing.

I think it varies based on the viewer and on the head repo:

viewer ⬇️ head:
viewer’s own repo
head:
owner’s repo
head:
other repo
owner origin $author
collaborator origin origin or upstream $author
3rd party origin origin or upstream $author

Notes:

  • there are 2 "or"s, but I crossed the less likely one; we can probably use the other option in our logic
  • when it's origin, there shouldn't be a git remote add

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

Successfully merging this pull request may close these issues.

Can RG recommend me the command to fetch a PR locally?
3 participants