Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Wikipedia: Wikitech

5 tips to get your code reviewed faster

 

 

Wikipedia wikitech RSS feed   Index | Next | Previous | View Threaded


sumanah at wikimedia

Aug 29, 2012, 2:55 PM

Post #1 of 10 (2205 views)
Permalink
5 tips to get your code reviewed faster

Version with helpful links:
https://www.mediawiki.org/wiki/Git/Code_review/Getting_reviews


1) Write small commits.

It's easier for other people to review small changes that only change
one thing. We'd rather see five small commits than one big one.

2) Respond to test failures and feedback.

Check your Gerrit settings and make sure you're getting email
notifications. If your code fails automated tests, or you got some
review already, respond to it in a comment or resubmission. Or hit the
Abandon button to remove your commit from the review queue while you
revise it.

(To see why automated tests fail, click on the link in the "failed"
comment in Gerrit, hover over the failed test's red dot, wait for the
popup to show, and then click "console output.")

3) Don't mix rebases with changes.

When rebasing, only rebase. That makes it easier to use the "Old Version
History" dropdown, which greatly quickens reviews. If non-rebase changes
are made inside a rebase changeset, you have to read through a lot more
code to find it and it's non-obvious.

4) Add reviewers.

I try to help with this. If I notice an unreviewed changeset lingering,
then I add a review request or two. (These are requests -- there's no
way to assign a review to someone in Gerrit.) But it's faster if you do
it right after committing. Some tricks:

* Click the name of the repository ("Gerrit project"), e.g.
operations/debs/squid , and remove "status:open" from the search box to
find other changesets in that repository. The people who write and
review those changesets would be good candidates to add as reviewers.
* Search through other commit summaries and changesets. Example:
Matmarex and Foxtrott are interested in reviewing frontend changes, so I
search for "message:css" to find changesets that mention CSS in their
commit summaries to add them to. You can use this and regexes to find
changes that touch the same components you're touching, to find likely
reviewers. Learn more at
https://gerrit.wikimedia.org/r/Documentation/user-search.html .

5) Review more.

Many eyes make bugs shallow. Read the code review guide and help out
with comments, "+1", and "-1". Those are nonbinding, won't cause merges
or rejections, and have no formal effect on the code review. But you'll
learn, gain reputation, and get people to return the favor by reviewing
you in the future. "How to review code in Gerrit" has the step-by-step
explanation. Example Gerrit search for MediaWiki commits that have not
had +1, +2, -1, or -2 reviews yet:
https://gerrit.wikimedia.org/r/#/q/-CodeReview%252B1+-CodeReview%252B2+-CodeReview-1+-CodeReview-2+project:%255Emediawiki.*,n,z


--
Sumana Harihareswara
Engineering Community Manager
Wikimedia Foundation

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


mail at tgries

Aug 29, 2012, 3:05 PM

Post #2 of 10 (2151 views)
Permalink
Re: 5 tips to get your code reviewed faster [In reply to]

Am 29.08.2012 23:55, schrieb Sumana Harihareswara:
> Version with helpful links:
> https://www.mediawiki.org/wiki/Git/Code_review/Getting_reviews
>
>
> 1) Write small commits.
>
> It's easier for other people to review small changes that only change
> one thing. We'd rather see five small commits than one big one.
>
> 2) Respond to test failures and feedback.
>
> Check your Gerrit settings and make sure you're getting email
> notifications. If your code fails automated tests, or you got some
> review already, respond to it in a comment or resubmission. Or hit the
> Abandon button to remove your commit from the review queue while you
> revise it.
>
> (To see why automated tests fail, click on the link in the "failed"
> comment in Gerrit, hover over the failed test's red dot, wait for the
> popup to show, and then click "console output.")
>
> 3) Don't mix rebases with changes.
>
> When rebasing, only rebase. That makes it easier to use the "Old Version
> History" dropdown, which greatly quickens reviews. If non-rebase changes
> are made inside a rebase changeset, you have to read through a lot more
> code to find it and it's non-obvious.
>
> 4) Add reviewers.
>
> I try to help with this. If I notice an unreviewed changeset lingering,
> then I add a review request or two. (These are requests -- there's no
> way to assign a review to someone in Gerrit.) But it's faster if you do
> it right after committing. Some tricks:
>
> * Click the name of the repository ("Gerrit project"), e.g.
> operations/debs/squid , and remove "status:open" from the search box to
> find other changesets in that repository. The people who write and
> review those changesets would be good candidates to add as reviewers.
> * Search through other commit summaries and changesets. Example:
> Matmarex and Foxtrott are interested in reviewing frontend changes, so I
> search for "message:css" to find changesets that mention CSS in their
> commit summaries to add them to. You can use this and regexes to find
> changes that touch the same components you're touching, to find likely
> reviewers. Learn more at
> https://gerrit.wikimedia.org/r/Documentation/user-search.html .
>
> 5) Review more.
>
> Many eyes make bugs shallow. Read the code review guide and help out
> with comments, "+1", and "-1". Those are nonbinding, won't cause merges
> or rejections, and have no formal effect on the code review. But you'll
> learn, gain reputation, and get people to return the favor by reviewing
> you in the future. "How to review code in Gerrit" has the step-by-step
> explanation. Example Gerrit search for MediaWiki commits that have not
> had +1, +2, -1, or -2 reviews yet:
> https://gerrit.wikimedia.org/r/#/q/-CodeReview%252B1+-CodeReview%252B2+-CodeReview-1+-CodeReview-2+project:%255Emediawiki.*,n,z
>
>
Something which needs so much text, is broken is some respect.
"Mies van der Rohe: Less is more."

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


hashar+wmf at free

Aug 30, 2012, 12:45 AM

Post #3 of 10 (2114 views)
Permalink
Re: 5 tips to get your code reviewed faster [In reply to]

Le 30/08/12 00:05, Thomas Gries a écrit :
> Something which needs so much text, is broken is some respect.
> "Mies van der Rohe: Less is more."

Well we probably nickname the community team "too long did not read" for
a reason, their job is indeed to write full reports which I myself find
enjoying. Tip for the hurry people: only read the titles :-)

--
Antoine "hashar" Musso


_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


hashar+wmf at free

Aug 30, 2012, 12:59 AM

Post #4 of 10 (2109 views)
Permalink
Re: 5 tips to get your code reviewed faster [In reply to]

Le 29/08/12 23:55, Sumana Harihareswara wrote:
> 1) Write small commits.

I cant stress how important this is. git has several ways to split a commit:
- git rebase --interactive <parent commit sha1>
- reset to master and git cherry-pick --no-commit <sha1> then use git
add --patch to select the hunk to craft a new small commit.
--> http://nuclearsquid.com/writings/git-add/

<snip>
> 3) Don't mix rebases with changes.
>
> When rebasing, only rebase. That makes it easier to use the "Old Version
> History" dropdown, which greatly quickens reviews. If non-rebase changes
> are made inside a rebase changeset, you have to read through a lot more
> code to find it and it's non-obvious.

The way to go is usually to git rebase and send that then do the new
change. That make reviewing ten times easier.

It is also a good practice to add a cover message on the new patchset to
explain what it changes compared to the previous one. For example:

PS3: rebased on latest master
PS4: fix the, now obsolete, function() call

Where PS is used instead of "PatchSet".

> 4) Add reviewers.

If you do not know who could review your change, do ask in #mediawiki or
#wikimedia-dev. People with a long experience will be able to tell you
who might be able to help.

> 5) Review more.

Asking a question about code, commenting about anything (whitespaces,
code you do not understand or dont like), doing +1 / -1, is NEVER going
to harm anyone. I have seen reviews from newbie that were not
understanding some part, and it ended up being a bug.
Reading code is a great way to learn the framework, asking question is
even better :)

--
Antoine "hashar" Musso


_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


krinklemail at gmail

Aug 30, 2012, 4:25 PM

Post #5 of 10 (2110 views)
Permalink
Re: 5 tips to get your code reviewed faster [In reply to]

On Aug 30, 2012, at 9:59 AM, Antoine Musso <hashar+wmf [at] free> wrote:

> It is also a good practice to add a cover message on the new patchset to
> explain what it changes compared to the previous one.

Yes, very important. If you submit a patch set, please do leave a quick comment
explaining what you changed. I personally like to use bullet points for those
comments like:

* Rebased

or

* Addressed comments from John.
* Removed redundant code in foo.js.

>
> PS3: rebased on latest master
> PS4: fix the, now obsolete, function() call
>
> Where PS is used instead of "PatchSet".
>

This is not needed. Because if you leave a comment (and do it right, as in,
click "Review" on the main gerrit change page under the correct "Patch set"
heading) gerrit prefixes this to your comments automatically.

The only reason to need such a prefix is if you're putting it somewhere else,
such as the commit-msg – where such comments don't belong in the first place.

Putting it in the commit message is imho annoying because:
* It is not at all helpful when the commit is merged because only the last
version is merged. The rest are in-between steps during the review process and
discussion and details of that process belong in the review thread, not in the
commit message.
* And, as the author, it is kinda hard because you don't know the patch version
number until it is submitted, and someone else can submit a version while you're
working.

Having said that, do make sure that your commit message is still accurate and
explains the full change (not just the first version of the patch, nor just the
last amendment to the patch).

-- Krinkle


_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


z at mzmcbride

Sep 4, 2012, 4:38 PM

Post #6 of 10 (2087 views)
Permalink
Re: 5 tips to get your code reviewed faster [In reply to]

Antoine Musso wrote:
> Le 29/08/12 23:55, Sumana Harihareswara wrote:
>> 1) Write small commits.
>
> I cant stress how important this is. git has several ways to split a commit:
> - git rebase --interactive <parent commit sha1>
> - reset to master and git cherry-pick --no-commit <sha1> then use git
> add --patch to select the hunk to craft a new small commit.
> --> http://nuclearsquid.com/writings/git-add/

In a previous wikitech-l thread from April 2012, Tim Starling wrote:

> Larger things with more benefits tend to get a higher priority than
> smaller things. So it's usually quicker to get 1500 lines of code
> reviewed than 15.

This seems to be a nasty discrepancy.

Of course these are only snippets from two threads, but the underlying idea
here seems to be that some reviewers prefer large branches of code that can
be reviewed all at once rather than a lot of small commits. And some
committers similarly prefer a "touch it once" approach (particularly if
there are several bugs related to the same area of code) over a bunch of
commits, for a variety of reasons, some of which are related to the
annoyance of Gerrit (Git?) dependency linking and others of which are
related to varying ideas of what's most efficient/easiest/sanest.

I think some kind of reconciliation is needed here in the advice to
committers, new and old. I guess whether to split commits up or not depends
on context?

MZMcBride



_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


mah at everybody

Sep 4, 2012, 5:31 PM

Post #7 of 10 (2085 views)
Permalink
Re: 5 tips to get your code reviewed faster [In reply to]

On 09/04/2012 07:38 PM, MZMcBride wrote:
> Antoine Musso wrote:
>> Le 29/08/12 23:55, Sumana Harihareswara wrote:
>>> 1) Write small commits.
>>
>> I cant stress how important this is. git has several ways to split a commit:
>> - git rebase --interactive <parent commit sha1>
>> - reset to master and git cherry-pick --no-commit <sha1> then use git
>> add --patch to select the hunk to craft a new small commit.
>> --> http://nuclearsquid.com/writings/git-add/
>
> In a previous wikitech-l thread from April 2012, Tim Starling wrote:
>
>> Larger things with more benefits tend to get a higher priority than
>> smaller things. So it's usually quicker to get 1500 lines of code
>> reviewed than 15.
>
> This seems to be a nasty discrepancy.
...
> I think some kind of reconciliation is needed here in the advice to
> committers, new and old. I guess whether to split commits up or not depends
> on context?

Or maybe these simply the differences in the sorts of reviews that
people like to do? Or maybe its a bit of both?

"If this is a major change you really want Tim to review, then make a
single big commit. If you want Krinkle to review your code, don't batch
it up into several seemingly unrelated changes in a big blob."

Because Sumana's initial instructions weren't about any particular
developer, maybe this is just something she has noticed as a tendency?

Mark.

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


sumanah at wikimedia

Sep 4, 2012, 5:57 PM

Post #8 of 10 (2070 views)
Permalink
Re: 5 tips to get your code reviewed faster [In reply to]

On 09/04/2012 05:31 PM, Mark A. Hershberger wrote:
> On 09/04/2012 07:38 PM, MZMcBride wrote:

>> I think some kind of reconciliation is needed here in the advice to
>> committers, new and old. I guess whether to split commits up or not depends
>> on context?
>
> Or maybe these simply the differences in the sorts of reviews that
> people like to do? Or maybe its a bit of both?
>
> "If this is a major change you really want Tim to review, then make a
> single big commit. If you want Krinkle to review your code, don't batch
> it up into several seemingly unrelated changes in a big blob."
>
> Because Sumana's initial instructions weren't about any particular
> developer, maybe this is just something she has noticed as a tendency?
>
> Mark.

That particular suggestion -- writing small commits -- came from Daniel
Kinzler, who gave feedback on the draft (see 19:55:45 in
http://bots.wmflabs.org/~wm-bot/logs/%23mediawiki/20120828.txt ). I
believe I have also heard similar advice from others.

I would love more clarifications from developers to help people decide
when to lump commits together into a changeset and when to split things up.

--
Sumana Harihareswara
Engineering Community Manager
Wikimedia Foundation

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


robla at wikimedia

Sep 4, 2012, 6:00 PM

Post #9 of 10 (2073 views)
Permalink
Re: 5 tips to get your code reviewed faster [In reply to]

On Tue, Sep 4, 2012 at 5:31 PM, Mark A. Hershberger <mah [at] everybody> wrote:
> Or maybe these simply the differences in the sorts of reviews that
> people like to do? Or maybe its a bit of both?

I think you're right that stylistic differences are at play.

One possible bit of guidance we can give is "make things as simple as
possible, but no simpler". That is, having a large change broken up
into smaller bits can sometime mean creating a jigsaw puzzle for the
reviewer to piece together prior to reviewing your big change.

Nonetheless, I know that, for example, some reviewers prefer to see
things broken up, and others don't. An example might be a big change
that requires a new API. Some reviewers don't mind reviewing that API
in isolation, whereas other reviewers want to see the calling code.
If the API is an obvious and welcome change that can stand on its own,
then that's going to be the better strategy. However, if the calling
code could have been done differently (and much better) with a
different API, then refactoring might be made harder by the fact that
there are now people depending on the previously reviewed/committed
API.

I think most reviewers agree that several unrelated changes bundled up
into the Omnibus MediaWiki Cleanup and Terrorism Prevention Commit of
2012 is a bad thing. And no, those reviewers are not in league with
terrorists.

Rob

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


rkaldari at wikimedia

Sep 4, 2012, 10:40 PM

Post #10 of 10 (2079 views)
Permalink
Re: 5 tips to get your code reviewed faster [In reply to]

On 9/4/12 5:57 PM, Sumana Harihareswara wrote:
> I would love more clarifications from developers to help people decide
> when to lump commits together into a changeset and when to split things up.

If your commits are going to be touching the same files repeatedly,
bundle them up into one large commit (using either --amend or squashing
after the fact).

If your commits are going to be touching separate files and there's not
a lot of dependency between them, it's probably best to keep them as
smaller discreet commits.

Ryan Kaldari

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Wikipedia wikitech RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.