6 ms·

How to write the perfect pull request (2015)

174 points by xiaodai 7y ago

airstrike 7y ago
> Use emoji to clarify tone. Compare “:sparkles: :sparkles: Looks good :+1: :sparkles: :sparkles:” to “Looks good.” Or, you know, write "Looks great!" Exclamation mark is the OG emoji
- Kwantuum 7y ago
  Or formalize phrases. The Google review guide [1] defines some phrases and as such there is no question about the tone of it. Prefix comments on non-essential parts of the commit with "Nit:", approve the changes with "LGTM" (looks good to me). [1] https://google.github.io/eng-practices/review/ https://google.github.io/eng-practices/review/
  - xvector 7y ago
    That's boring. As a form of protest, I'm going to do all future reviews in emoji only.
    - dharmab 7y ago
      On smaller projects I've done code reviews with reaction GIFs (although always with people who knew me and my humour well).
chenhan 7y ago
This is a good article that covers how to communicate in a pull request. I think there are two other essentials in making good pull requests. 1. Good commit messages. Follow this guide: https://chris.beams.io/posts/git-commit/ https://chris.beams.io/posts/git-commit/ 2. Following the pull request workflow correctly. Follow this guide: https://github.com/susam/gitpr https://github.com/susam/gitpr Writing good and consistent commit messages make the commit log easy to read and search. Pull request workflow is equally important. It is kind of a rules of engagement that co-developers follow to keep working on their stuff concurrently. It keeps unnecessary merges to minimum and keeps the commit history clean. By following these two things, you are not only going to be nice to your co-developers but you are going to be nice to your managers and release departments too who could look at your commit log and figure out what bugs were fixed and what features were released.
- chenhan 7y ago
  I will elaborate why I think the two points in my comment above are important. Git Rebase, Git bisect and other operations display the commit summary line when they get stuck with merge conflicts or find an issue, so I find good commit summary lines very helpful during those operations. Without good commit messages, resolving issues during those operations can get confusing. This is one of the reasons why writing good commit messages are important. I see many developers working on their PR branch and constantly merging new commits from master into the PR branch as the master keeps moving ahead. It creates a big mess of merges in both directions-- (A) from master to PR branch during development and (B) later again from PR branch to master when the PR gets merged. The first set of merges from master to PR branch are totally unnecessary. It adds nothing meaningful to the commit history. They are just extra commits to scroll through while looking at git log. Every time the master branch moves ahead, just rebase your PR branch on master. The commit history remains clean and minimal. A good PR workflow teaches you that. Having said that, merge commits from PR branch to master are totally fine. They do add something meaningful. They show the point at which a PR was merged into the main project.
  - dimes 7y ago
    If you share a development branch with someone, then you should prefer merging over rebasing because rebasing changes rewrites the commit history, causing the locally checked out branches among collaborators to disagree. You never know when someone will need to take your branch to develop on, so generally it’s a good idea to merge over rebase. When master is merged into your PR branch, the fork point is forwarded to the last master commit, so you shouldn’t have any trouble merging back into master.
    - pm90 7y ago
      I think the OPs concern was more with the commit history in the PR being polluted with merge commits. I do agree that merge commits can obfuscate history somewhat. However, I agree with dimes that its better to not rewrite history just to keep the commit history clean... the cleanliness is not worth the price for inability to collaborate effectively. Also, any professional code review tool will not let merge commits affect your review. Highly recommend reviewable.io for this.
matheusmoreira 7y ago
Better support for pull request revision would be nice. In mailing lists, the entire patch set is resent when revisions are made. When I force push revised commits to my branch, the old commits are eventually garbage collected despite the existence of references to those commits in the pull request. Also, it is important to discuss pull request etiquette. When is it appropriate to mention the maintainers? How to draw attention to a pull request?
- chenhan 7y ago
  It is a problem in mailing lists but GitHub has good tooling around it. Every time you force-push, GitHub keeps and provides direct links to the old commit, the new commit and the diff between the two commits. Every force push appears as its own diff on the GitHub PR page. How is GitLab doing in this area? Does GitLab show diffs for force-pushes?
  - matheusmoreira 7y ago
    > Every time you force-push, GitHub keeps and provides direct links to the old commit, the new commit and the diff between the two commits. Not in my experience. Here's one of my pull requests: https://github.com/mchehab/zbar/pull/64 https://github.com/mchehab/zbar/pull/64 The maintainer reviewed some of the changes and I revised my commits as a result. The review is correctly marked as outdated. Clicking on the file name tells me the commit cannot be found. > We went looking everywhere, but couldn’t find those commits. > Sometimes commits can disappear after a force-push. Head back to the latest changes here.
    - chenhan 7y ago
      Your pull request shows exactly what I am talking about. For every force-push it has direct links to the old commit, the new commit and the diff. Pick the first force push in your PR. It says > matheusmoreira force-pushed the matheusmoreira:binary-decoding branch from aec04b3 to 87a0b3c on 5 Nov 2019 Click on 'force-pushed'. That's the diff of force-push. Click on 'aec04b3'. That's the commit before force-push. Click on '87a0b3c'. That's the new commit in the force-push.
dang 7y ago
Discussed at the time: https://news.ycombinator.com/item?id=9717937 https://news.ycombinator.com/item?id=9717937
- Drakar1903 7y ago
  But can absolutely be revisited.
teddyh 7y ago
> @mention Yes, GitHub would love for their namespace to be the implied namespace in commit messages. In other words, please don't. Use names or e-mail addresses to refer to people.
- peterwwillis 7y ago
  ....but GitHub mentions are actually useful, and send notifications to users to alert them to look at the post.
  - uranusjr 7y ago
    GitHub mentions are good—in the pull request thread you open on GitHub. In commit messages, not so much. The same rule applies for issue references; URLs should be preferred (or at least something like GH-XXXX) to simple hashtag numbers in commit messages, the improvement matters a lot when it does, but you can only get it if you do it from the beginning (Git commits being immutable).
  - deepersprout 7y ago
    Put them in comments on the commit on github. It's better to keep github stuff inside github. Your source code with your git history may move off github one day.
- sweeneyrod 7y ago
  They're talking about pull requests, not commits.
bilekas 7y ago
> Use emoji to clarify tone. Compare “:sparkles: :sparkles: Looks good :+1: :sparkles: :sparkles:” to “Looks good.” .. Just approve that damn thing. Don't decorate it in confetti ;)
- klohto 7y ago
  “Be careful to not use only one sparkle as the maintainer might get offended”
tommit 7y ago
I was lucky enough to have a team member introduce me to this article early in my career: https://mtlynch.io/human-code-reviews-1/ https://mtlynch.io/human-code-reviews-1/ I still think about it when reviewing code, it's really sound advice over all
davidrm 7y ago
> Ask, don’t tell. (“What do you think about trying…?” rather than “Don’t do…”) When I led a team of 6 people, I did the opposite. The guideline was to always use the imperative form if you knew there was a better way, especially if there's a precedent or it's a written rule in a guideline, but even for opinionated things. It resulted in clear debate in PRs, ZERO personal conflicts, everything was very civil and productive. I'm just mentioning this as a personal anecdote, not suggesting that there's a right or wrong way. The reason this worked in my team is because we were, and still are - a team. They're still doing it like that even though I'm no longer involved in day to day.
- datanerd 7y ago
  > The guideline was to always use the imperative form if you knew there was a better way, especially if there's a precedent or it's a written rule in a guideline, but even for opinionated things. Same here. We used imperative form too and never had any problems. All developers were matured enough to not take comments about technical stuff personally. I think as long as we are being direct and polite in the reviews, there should be no issues.
- hcarvalhoalves 7y ago
  I agree. It shouldn't disguise as a question if you're not really inviting a debate, doesn't feel honest IMO. When the imperative form is used the reader will often assume you know something they don't, and only argue when they see a problem or want to validate the assumption. I believe the more the comments (annotations?) are about the code itself ("This should do X", "This is not doing Y") rather than a back-and-forth conversation ("What do you think about X?", "Have you thought about X?"), the less personal it feels, and the more the ego gets out of the way. Same thing for denying PRs with "request changes" - I've had a few people say they avoid doing it because it feels aggressive, but knowing there's a problem and still letting it into production (and then have the PR author fix a much more costly mistake) isn't honest or professional.
  - ProZsolt 7y ago
    > the less personal it feels, and the more the ego gets out of the way. That's why I usually use "We should do this" instead of "You should do this", and "Our code" not "Your code" because we are a team and we do this together. I have no problem denying PRs or if somebody do this with mine. If we can't get to the same page, we just call the tech lead to break up the tie.
ddevault 7y ago
Rebase, rebase, rebase. Shameless plug for a tutorial I wrote: https://git-rebase.io https://git-rebase.io

174 points by xiaodai 7y ago

> Use emoji to clarify tone. Compare “:sparkles: :sparkles: Looks good :+1: :sparkles: :sparkles:” to “Looks good.” Or, you know, write "Looks great!" Exclamation mark is the OG emoji

Kwantuum 7y ago
Or formalize phrases. The Google review guide [1] defines some phrases and as such there is no question about the tone of it. Prefix comments on non-essential parts of the commit with "Nit:", approve the changes with "LGTM" (looks good to me). [1] https://google.github.io/eng-practices/review/ https://google.github.io/eng-practices/review/
- xvector 7y ago
  That's boring. As a form of protest, I'm going to do all future reviews in emoji only.
  - dharmab 7y ago
    On smaller projects I've done code reviews with reaction GIFs (although always with people who knew me and my humour well).

chenhan 7y ago

This is a good article that covers how to communicate in a pull request. I think there are two other essentials in making good pull requests. 1. Good commit messages. Follow this guide: https://chris.beams.io/posts/git-commit/ https://chris.beams.io/posts/git-commit/ 2. Following the pull request workflow correctly. Follow this guide: https://github.com/susam/gitpr https://github.com/susam/gitpr Writing good and consistent commit messages make the commit log easy to read and search. Pull request workflow is equally important. It is kind of a rules of engagement that co-developers follow to keep working on their stuff concurrently. It keeps unnecessary merges to minimum and keeps the commit history clean. By following these two things, you are not only going to be nice to your co-developers but you are going to be nice to your managers and release departments too who could look at your commit log and figure out what bugs were fixed and what features were released.

chenhan 7y ago
I will elaborate why I think the two points in my comment above are important. Git Rebase, Git bisect and other operations display the commit summary line when they get stuck with merge conflicts or find an issue, so I find good commit summary lines very helpful during those operations. Without good commit messages, resolving issues during those operations can get confusing. This is one of the reasons why writing good commit messages are important. I see many developers working on their PR branch and constantly merging new commits from master into the PR branch as the master keeps moving ahead. It creates a big mess of merges in both directions-- (A) from master to PR branch during development and (B) later again from PR branch to master when the PR gets merged. The first set of merges from master to PR branch are totally unnecessary. It adds nothing meaningful to the commit history. They are just extra commits to scroll through while looking at git log. Every time the master branch moves ahead, just rebase your PR branch on master. The commit history remains clean and minimal. A good PR workflow teaches you that. Having said that, merge commits from PR branch to master are totally fine. They do add something meaningful. They show the point at which a PR was merged into the main project.
- dimes 7y ago
  If you share a development branch with someone, then you should prefer merging over rebasing because rebasing changes rewrites the commit history, causing the locally checked out branches among collaborators to disagree. You never know when someone will need to take your branch to develop on, so generally it’s a good idea to merge over rebase. When master is merged into your PR branch, the fork point is forwarded to the last master commit, so you shouldn’t have any trouble merging back into master.
  - pm90 7y ago
    I think the OPs concern was more with the commit history in the PR being polluted with merge commits. I do agree that merge commits can obfuscate history somewhat. However, I agree with dimes that its better to not rewrite history just to keep the commit history clean... the cleanliness is not worth the price for inability to collaborate effectively. Also, any professional code review tool will not let merge commits affect your review. Highly recommend reviewable.io for this.

matheusmoreira 7y ago

Better support for pull request revision would be nice. In mailing lists, the entire patch set is resent when revisions are made. When I force push revised commits to my branch, the old commits are eventually garbage collected despite the existence of references to those commits in the pull request. Also, it is important to discuss pull request etiquette. When is it appropriate to mention the maintainers? How to draw attention to a pull request?

chenhan 7y ago
It is a problem in mailing lists but GitHub has good tooling around it. Every time you force-push, GitHub keeps and provides direct links to the old commit, the new commit and the diff between the two commits. Every force push appears as its own diff on the GitHub PR page. How is GitLab doing in this area? Does GitLab show diffs for force-pushes?
- matheusmoreira 7y ago
  > Every time you force-push, GitHub keeps and provides direct links to the old commit, the new commit and the diff between the two commits. Not in my experience. Here's one of my pull requests: https://github.com/mchehab/zbar/pull/64 https://github.com/mchehab/zbar/pull/64 The maintainer reviewed some of the changes and I revised my commits as a result. The review is correctly marked as outdated. Clicking on the file name tells me the commit cannot be found. > We went looking everywhere, but couldn’t find those commits. > Sometimes commits can disappear after a force-push. Head back to the latest changes here.
  - chenhan 7y ago
    Your pull request shows exactly what I am talking about. For every force-push it has direct links to the old commit, the new commit and the diff. Pick the first force push in your PR. It says > matheusmoreira force-pushed the matheusmoreira:binary-decoding branch from aec04b3 to 87a0b3c on 5 Nov 2019 Click on 'force-pushed'. That's the diff of force-push. Click on 'aec04b3'. That's the commit before force-push. Click on '87a0b3c'. That's the new commit in the force-push.

dang 7y ago

Discussed at the time: https://news.ycombinator.com/item?id=9717937 https://news.ycombinator.com/item?id=9717937

Drakar1903 7y ago
But can absolutely be revisited.

teddyh 7y ago

> @mention Yes, GitHub would love for their namespace to be the implied namespace in commit messages. In other words, please don't. Use names or e-mail addresses to refer to people.

peterwwillis 7y ago
....but GitHub mentions are actually useful, and send notifications to users to alert them to look at the post.
- uranusjr 7y ago
  GitHub mentions are good—in the pull request thread you open on GitHub. In commit messages, not so much. The same rule applies for issue references; URLs should be preferred (or at least something like GH-XXXX) to simple hashtag numbers in commit messages, the improvement matters a lot when it does, but you can only get it if you do it from the beginning (Git commits being immutable).
- deepersprout 7y ago
  Put them in comments on the commit on github. It's better to keep github stuff inside github. Your source code with your git history may move off github one day.
sweeneyrod 7y ago
They're talking about pull requests, not commits.

bilekas 7y ago

> Use emoji to clarify tone. Compare “:sparkles: :sparkles: Looks good :+1: :sparkles: :sparkles:” to “Looks good.” .. Just approve that damn thing. Don't decorate it in confetti ;)

klohto 7y ago
“Be careful to not use only one sparkle as the maintainer might get offended”

tommit 7y ago

I was lucky enough to have a team member introduce me to this article early in my career: https://mtlynch.io/human-code-reviews-1/ https://mtlynch.io/human-code-reviews-1/ I still think about it when reviewing code, it's really sound advice over all

davidrm 7y ago

> Ask, don’t tell. (“What do you think about trying…?” rather than “Don’t do…”) When I led a team of 6 people, I did the opposite. The guideline was to always use the imperative form if you knew there was a better way, especially if there's a precedent or it's a written rule in a guideline, but even for opinionated things. It resulted in clear debate in PRs, ZERO personal conflicts, everything was very civil and productive. I'm just mentioning this as a personal anecdote, not suggesting that there's a right or wrong way. The reason this worked in my team is because we were, and still are - a team. They're still doing it like that even though I'm no longer involved in day to day.

datanerd 7y ago
> The guideline was to always use the imperative form if you knew there was a better way, especially if there's a precedent or it's a written rule in a guideline, but even for opinionated things. Same here. We used imperative form too and never had any problems. All developers were matured enough to not take comments about technical stuff personally. I think as long as we are being direct and polite in the reviews, there should be no issues.
hcarvalhoalves 7y ago
I agree. It shouldn't disguise as a question if you're not really inviting a debate, doesn't feel honest IMO. When the imperative form is used the reader will often assume you know something they don't, and only argue when they see a problem or want to validate the assumption. I believe the more the comments (annotations?) are about the code itself ("This should do X", "This is not doing Y") rather than a back-and-forth conversation ("What do you think about X?", "Have you thought about X?"), the less personal it feels, and the more the ego gets out of the way. Same thing for denying PRs with "request changes" - I've had a few people say they avoid doing it because it feels aggressive, but knowing there's a problem and still letting it into production (and then have the PR author fix a much more costly mistake) isn't honest or professional.
- ProZsolt 7y ago
  > the less personal it feels, and the more the ego gets out of the way. That's why I usually use "We should do this" instead of "You should do this", and "Our code" not "Your code" because we are a team and we do this together. I have no problem denying PRs or if somebody do this with mine. If we can't get to the same page, we just call the tech lead to break up the tie.

ddevault 7y ago

Rebase, rebase, rebase. Shameless plug for a tutorial I wrote: https://git-rebase.io https://git-rebase.io