[owncloud-devel] Pull Requests suggested guidelines

Morris Jobke morris at owncloud.com
Wed May 21 21:29:31 GMT 2014


I just want to add another point:

2014-03-17 10:21 GMT+01:00 Vincent Petry <pvince81 at owncloud.com>:
> Hi,
> I wrote a small list of guidelines about Pull Requests which I think
> should help improve the reviewing process a bit:
> 1. From Tanghus' comment on a calendar app PR: "When making a PR always
> limit it to a single issue, or it will get stuck for review very likely
> until it's too outdated to ever get merged."
> 2. Summarize what the PR does to make it easier to understand for people
> who don't know that part of the code/app. Sometimes one needs to read a
> long issue thread before they can understand what it is about, which is
> usually discouraging for reviewers/testers.
> 3. Add test steps to your PRs with checkboxes, it makes it much quicker
> to understand what needs to be tested. It also helps you make sure you
> covered all cases.
> I usually write the test cases and test them myself, ticking the boxes.
> Then reviewers can just go through the same steps to make sure it works
> on their side.
> Also, writing down the steps confirms that you've tested it. (you do
> test your changes, do you ? ;-))
> These also help when testing backports.
> 4. When you start working on a big task, open a PR with the prefix
> "[WIP]" and a nice :construction: emoji. This helps making other people
> aware of your progress and gives them a chance to:
>    a) comment earlier on the said change, in case you might be heading
> in the wrong direction
>    b) makes it possible to spot potential conflicts earlier
>    c) prevent duplicate work in case someone already did this
>    d) collaborate (some people helped me test an unfinished PR, which
> was nice!)
>    e) cheer you up for the progress ;-)
> 5. If you know who is familiar with a given part of the code, you can
> mention them in the PR.

6. Add a Kanban label: Either "5- To review" if the PR is ready for review
doesn't need any changes or "4 - Developing" if it's a work in progress PR
there were issues and comments which aren't resolved/implemented. An keep in
mind to update the label.

Then it's easier to find an appropiate PR if you've got time for a review
doesn't have to search for a "ready to review" PR - you just need to filter
for it [1].

I've done it for the first page of PRs in the last days, when I checked them


[1] https://github.com/owncloud/core/issues?labels=5+-+To+review
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.owncloud.org/pipermail/devel/attachments/20140521/cc347c53/attachment.html>

More information about the Devel mailing list