-
Notifications
You must be signed in to change notification settings - Fork 538
How to do PRs that are gud
How to do [Pull Requests] (https://help.github.com/articles/using-pull-requests/) that are gud!
(Closely related: how to do code reviews)
Meaningful short summary of WHY this PR
- Good - fix kv579 dataloss bug with DVVSets
- Good - Use multipaxos for per key SC
- Bad - fix bug (doesn't tell me anything)
- Bad - remove anon fun (the diff tells me this, why did you need to commit this though??)
- Good - Steps to reproduce (if you have them)
- Best - A TEST that shows the bug in the unpatched code
- OK - A description of the bad behaviour and WHY you can't provide the above
If there is an Issue that provides the above, then linking to it is enough. If the issue is a long, rambling, conversation/exploration, please summarise.
What the feature does, how it can be used, examples.
You can clarify WHAT/HOW you did (should be clear from the diff, right?) if you wish after you've covered why.
And also:
- Nominate reviewers and others who may or should be appraised of the proceedings, by mentioning them with an
@. - Link in other PRs, if any, that need to be merged first, and those which depend on the one being opened.
- Mention the Jira issue if appropriate.
- List the riak_test modules that may be affected by the change. Fixing tests after your PR gets approved and merged is not fun.
Prefer creating branches in the company repo instead of your own (forked) repo. This will facilitate collaboration as others will be able to push more commits in it.
-
If not many eyes have seen your PR yet, consider squashing multiple WIPs into granular, well-formed and properly documented commits, and push with force.
-
When, after a round of reviews, all issues have been resolved, and the PR eventually approved, consider squashing fixes into the main commit. Again, this will reduce litter in the history and improve commit granularity, and make reverting of this commit, if need be, less of a hassle. At this stage, however, a reapproval may be desirable.
Hint:
If the branch contains a single commit, github will use its title and commit message to populate the PR title and description for you.
It's best to avoid conflicts from accruing in your branch; one way to do that is to file an intermediate PR, separately from the feature work, and preferably at an early stage. When you are already late, and there be aggravating circumstances where sources with your changes have been renamed in the main branch (or parts of the source containing your edits moved to another file), there are a couple of things to observe:
- Study the merges that have happened in develop since you have branched off. If a part of file1.erl you are editing in your branch, has been moved to file2.erl -- and edited there subsequently in turn -- by someone else, you will have to copy and paste modified bits from both sources. The diff will show a massive deletion in file1.erl and massive addition in file2.erl, with no conflict whatsoever, and without knowing what changed where, it's easy to let old code slip into the synthesized merge commit. This has happened before: https://github.com/basho/riak_kv/pull/1400.
- Use a 3-way merge tool (meld, for example). Use it judiciously: it will not help in the concurrent-edits-post-rename situation detailed above.
If a rebase on develop threatens to be dangerously complex (specifically, more complex than a merge), consider reassembling commits de novo:
- merge develop into your branch first;
- create a separate branch (say,
feature-copy), which will contain the original commit history for historical purposes; - do
git reset --mixed origin/your-feature-branch, and usegit citoolto compile new commits on a by-chunk or by-line basis; - once you have cleared your work tree and staging area, the resulting history is not only mergeable back into develop, but it can also be cleanly rebased onto it;
- to verify, do
git diff HEAD feature-copy; - push both the newly reassembled branch (the one underlying you PR) as well as
feature-copy.
The real story of repeated attempts to land a big, two-month long PR, see https://github.com/basho/riak_kv/pull/1398 (and the numerous PRs linked therein of the same content that were withdrawn).
The same goes. If you raise an issue that you know how to reproduce, please provide a test or at least copy pastable steps to reproduce, this makes much better use of your colleagues time.
Make sure there is no hanging whitespace and all tabs are expanded to spaces. Emacs users will find this command useful: M-x whitespace-cleanup RET. Interesting variables and a special mode to highlight fixable whitespace can be customized by M-x customize-apropos RET trailing RET.