How to write the perfect pull request (2015) • Hacker News Board

Re: How to write the perfect pull request (2015)

#11

Post by dimes » Wed, Feb 19, 2020, 4:16 AM UTC

chenhan wrote: post #6

chenhan wrote: post #3

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/ 2. Following the pull request workflow correctly. Follow this guide: https://github.com/susam/gitpr Writing good and consistent commit messages make the commit log easy to read and search. Pull…

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 goo…

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.

Re: How to write the perfect pull request (2015)

#12

Post by pm90 » Wed, Feb 19, 2020, 7:39 AM UTC

dimes wrote: post #11

chenhan wrote: post #6

Earlier quoted context omitted.

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 goo…

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…

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.

Re: How to write the perfect pull request (2015)

#13

Post by teddyh » Wed, Feb 19, 2020, 7:45 AM UTC

> @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.

Re: How to write the perfect pull request (2015)

#14

Post by Kwantuum » Wed, Feb 19, 2020, 8:02 AM UTC

airstrike wrote: post #2

> 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

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/

Re: How to write the perfect pull request (2015)

#15

Post by Drakar1903 » Wed, Feb 19, 2020, 8:23 AM UTC

dang wrote: post #10

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

But can absolutely be revisited.

Re: How to write the perfect pull request (2015)

#16

Post by peterwwillis » Wed, Feb 19, 2020, 9:14 AM UTC

teddyh wrote: post #13

> @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.

....but GitHub mentions are actually useful, and send notifications to users to alert them to look at the post.

Re: How to write the perfect pull request (2015)

#17

Post by tommit » Wed, Feb 19, 2020, 9:42 AM UTC

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/

I still think about it when reviewing code, it's really sound advice over all

Re: How to write the perfect pull request (2015)

#18

Post by uranusjr » Wed, Feb 19, 2020, 10:05 AM UTC

peterwwillis wrote: post #16

teddyh wrote: post #13

> @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.

....but GitHub mentions are actually useful, and send notifications to users to alert them to look at the post.

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).

Re: How to write the perfect pull request (2015)

#19

Post by deepersprout » Wed, Feb 19, 2020, 10:11 AM UTC

peterwwillis wrote: post #16

teddyh wrote: post #13

> @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.

....but GitHub mentions are actually useful, and send notifications to users to alert them to look at the post.

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.

Re: How to write the perfect pull request (2015)

#20

Post by polyphonicist » Wed, Feb 19, 2020, 10:22 AM UTC

dimes wrote: post #11

chenhan wrote: post #6

Earlier quoted context omitted.

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 goo…

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…

> If you share a development branch with someone, then you should prefer merging over rebasing

Your parent comment is suggesting rebase only for pulling latest changes in master into your pull request.

For merging someone's pull request to the team's master, sure use a merge commit.

But if you are working on a pull request and while you are working on it, the team's master gets updated and now you want to base your work on the recent master, by all means, use git rebase. That is what it is meant for, to rebase your work on another work. It's in the name itself.

Right tool for the right job.