-
-
Notifications
You must be signed in to change notification settings - Fork 64
Guide: PR Excellence
For PR makers, reviewers, mergers, and all of them to level up
The Stronger Guidelines are things we really want to stick to. Anything else would be greatly appreciated.
PR stands for 'pull request'. Person 1 has a branch. They're asking (requesting) person 2 to git pull and merge those changes into a different branch. This 'different branch' might be on a fork, just a different repo, or a different branch on the same repo.
NOTE: The UI this doc describes is from 09/21/18.
- If no one who can review a PR has been around for a couple of weeks, or some other extreme situation comes up, anyone can merge a PR.
- Unless changes are requested, one approval means the PR maker can merge their own PR.
- Unless changes are requested, two approvals means anyone can merge a PR.
- Don't review or merge [WIP] PRs, though you can comment if the PR maker wants input.
- If there's a conversation going on in the PR that's been active sometime in the past week, please let that conversation play out before approving. If you're willing, go ahead and add your thoughts to that conversation.
- PR Makers
-
PR Reviewers
- Notice what's already going on
- Do actually review the PR
- Say yes more than you say no
- When should I merge
- Avoid blocking
- Don't approve, request changes for, or merge a WIP
- Travis tests
- The PR maker disappears
- I like the PR, but there's more I want to talk about
- Labels and projects
- Do I have to be requested?
- PR Mergers
Keep your PR focused on one thing. Don't change anything unrelated to your one purpose. If you're changing the color of a button, don't start fixing spelling mistakes. We all do it by accident sometimes, so just do your best.
PR early and often, even before your PR isn't ready to be merged. When you're first starting to work on a change, put [WIP] at the start of the PR title. [WIP] PRs won't be reviewed or merged, but it gives us access to the progress you've made (in case you get hit by a bus).
When you're ready for your PR to be reviewed and judged, remove the [WIP] from the title and ask for reviewers.
If you're a collaborator and your ready for your PR to be judged, make sure you're in the 'Conversation' tab and look in the right column of the PR at the top. You should see 'Reviewers' there. Click on the gear icon to add at least two reviewers to your PR. Right now (09/21/18), @ethanbb, @wacii, @knod, and @pporfilio are all good bets. Only the GitHub repo's set 'collaborators' can be reviewers.
NOTE: We're all volunteers and reviews might take a while to come in. If we take too long, do feel free to ping us with a comment in the PR or a message on Slack.
If your PR doesn't completely solve an existing issue, but is about that issue (or about a different PR), put EXACTLY '#' and then the issue or PR number in the title somewhere. For example: Begins the final stages of humanity's evolution - issue #149003. REMEMBER THE '#'. Github will automatically put a note in the issue or other PR that this PR is related to the issue or other PR.
If your PR finishes an issue, put EXACTLY 'Fixes #' and then the issue's number in the title somewhere. For example: Fixes ##149004 Triggers the singularity to save humanity from what it has become. REMEMBER THE '#'. If your PR is merged, Github will automatically close the issue and put a note that this PR did it.
You can do it even after you've turned in the PR by editing the title and adding it in, but it has to be done before the PR is merged.
If you forget to do it from the PR title and the PR has been closed (merged or turned down), editing the title won't fix it. Please go to the issue that your PR works on and put your PR's commit number in the comments of the issue. You can find your commit number at the bottom of the page of the merged PR, which is in the closed PRs. If it closes the issue, please say so and then close the issue.
If you're a collaborator, it'd be great if you could add labels to the PR, or show that this PR is related to an ongoing 'project' in the repo. It can help us see the big picture of what's been going on in the project. In the column on the right, click on the gear icons for 'Labels' and 'Projects' and see if there's anything that seems related to what your PR is doing.
- If someone has requested changes, don't approve of a PR, though definitely comment to let people know why you're thinking about the PR.
- If there's a conversation going on in the PR that's been active sometime in the past week, please let that conversation play out before approving. If you're willing, go ahead and add your thoughts to that conversation.
- Read the code that's been changed to see if it makes sense.
- Watch out for changes in package-lock.json. If the PR hasn't deliberately changed the packages, package-lock.json should not be changed.
- Pull it to a new branch on your local repo and try it out yourself or try it out through the nelify preview. (We haven't yet discussed whether netlify is enough on its own.)
Not that this has been a problem, but let's put it in words that all contributions are welcome and should be encouraged. If you don't object to what it does, then approve it. If you don't love it, but you're willing to compromise, it's good. If you don't like what it does, or if it has some serious problems, have a respectful conversation about it. Try to assume good intentions from everyone involved.
See the stuff about merging. If you don't want it to be merged yet, whatever it is, don't approve it and request that others hold off too.
We've all been guilty of blocking at some point or other. If a PR has some minor issues - it hasn't been linted, the comments have typos, you don't like the variable names, etc. - don't let it stop you from approving. You can fix those later if they're really bugging you. If it's not breaking the app's situation, fold it in.
'[WIP]' or 'WIP' stands for 'work in progress'. Don't review those or, if you do, just comment, don't approve or request changes. If you've already done the unthinkable, you can undo it by clicking on the reviews at the bottom of the 'Conversation' tab and dismissing your review.
Don't approve a PR that isn't passing the Travis CI tests, even if it's not the fault of the PR maker. Click on the Travis 'details' to see what the problem is. Maybe 'sign in with Github' (top right corner) and re-run the tests to see if it was just a temporary issue, like a timeout. If it's something more serious, do a review to ask for changes to the PR. If you think you know a bit about what's going on, or have some guesses, say so and describe what you're thinking. If you have no clue, say that instead.
Also, check whether the actual build itself is failing the Travis tests. If they built their PR on top of a failing build, that's not their fault. Still don't accept their PR - the repo's failing tests might be hiding the PR's own problems. Wait till you've fixed the problem in the repo, have them merge those changes, and then see if their build passes.
You've got a couple of options here. If it's not a critical PR, you can wait a while (two or three weeks if possible) to see if they manage to come back. Also, if it's an issue a person new to coding, or new to the project, can easily solve, you can give them the issue instead and they can start over if they want.
If you do want to keep the work that's already in the PR, you can always add the user's remote repo address to your local repo, pull their branch, do work on top of it, and make a new PR with it. If they've deleted their branch, or you don't like doing it that way, there's also a way to do it directly from the PR.
In that case, don't approve or request changes. You can review just to 'comment'. You can also just write comments in the thread. You can also bring it up in the Slack channel or in a meeting. If it makes sense, you can make an issue about it.
If a PR doesn't have a label and/or project yet, please add at least one.
No, you can be a reviewer even if no one requested you to be one. Do be respectful, though.
- When there's one or more approvals and you're the one who made the PR.
- When you see a PR that has two or more approvals.
- When there some strange situation. For example, everyone who could have given you a review hasn't been around for a couple of weeks.
- When the tests aren't passing, even if it's not because of this PR. See Travis tests - that same stuff applies here too.
- When someone else has requested changes.
- When the PR is a work in progress - it should have '[WIP]' in its title.
Don't do anything if it's a WIP. Otherwise, either the person who made the PR has to fix it and push again, or someone else has to resolve the conflict. If you do the second one, make sure you coordinate with the person who made the PR (unless they've disappeared). The person who made the PR will be out of sync with their branch after that and will have to pull before making any new changes to avoid new conflicts.
That said, if you're taking that option, there are a few ways to do it:
- Sometimes you can use Github to edit the code right there. The down-side is that you won't be able to test our your code before merging the change into the PR.
- Another way is to pull the PR into your local repo, fix the conflicts there and then...
- Well, push to your repo and make a new PR from your branch.
- Supposedly you can push to the PR instead, but I (knod) haven't been able to do that successfully yet.
- If the person who made the PR is very responsive, you can push to your branch then make a PR on their repo's branch. They can merge it in and their PR will show the changes.
Usually, it'll be a conflict with changes that were made on our dev branch. This is how you work it out on your local repo:
- Be on your PR branch
- Make sure there are no new changes on there
git checkout -b merge-testgit pull your-name-for-the-cfb-online-repo-link dev- See how hard the conflicts are to resolve.
- If you think you can take care of it as it is
- Go back to your PR branch.
git pull your-name-for-the-cfb-online-repo-link dev- Fix the conflicts
git add .git commitgit push origin my-PR-branch-name-that-I-made- You should be good!
- If you think you may be in a more complicated tangle, hit us up on Slack, in the PR's issue, or some other creative way.
If you're not the PR maker, just do the same basic stuff, but in combination with the other instructions given in number 2 in Merge conflicts.
It sounds a bit weird, but:
- Go to the branch of the PR.
- Make sure there aren't any changes on the branch.
- Find the SHA of the last commit where the file was the way it was supposed to be.
- Assuming the SHA was 'c5f567' and the file's path was '/p/n.ext', do
git checkout c5f567 -- /p/n.ext git add .git commit- Push the changes to the PR.
Based off of https://stackoverflow.com/a/215731
This will change the history of your dev branch, but since it's supposed to always match up with our repo's dev branch anyway, it shouldn't matter.
- If you have changes on dev that you want to save, just
- Check out a new branch: `git checkout -b branch-named-for-the-changes-you-want-to-make
- Checkout dev again:
git checkout dev
git fetch your-name-for-the-cfb-online-repo-link dev-
git reset --hard your-name-for-the-cfb-online-repo-link/dev(note the '/' in there just before dev) - That's it!