[3013] in BarnOwl Developers
Re: Thoughts on our current code review policy
daemon@ATHENA.MIT.EDU (Nelson Elhage)
Thu Mar 24 18:20:56 2011
Date: Thu, 24 Mar 2011 18:20:49 -0400
From: Nelson Elhage <nelhage@nelhage.com>
To: David A Benjamin <davidben@mit.edu>
Cc: Anders Kaseorg <andersk@mit.edu>, barnowl-dev@mit.edu
In-Reply-To: <alpine.DEB.1.10.1103241730150.25741@dr-wily.mit.edu>
Based on discussion here and on Zephyr, here are the conclusions I have:
- We should stop using email for code review. Instead of blocking on setting up
a gerrit instance, we'll try using pull requests on github and the github fork
queue.
Hopefully this should be easy for people to figure out. There is also a
command-line client to github, which I believe has support for generating pull
requests, at <https://github.com/defunkt/github-gem>.
- We should try to chill out about style issues, especially since the current
codebase is so inconsistent. If you are going to comment on style issues, try
to make it clear that it's a nitpick, and unless the issue is totally
egregious, it should probably typically be the author's choice whether to take
the suggestion or not.
If there is no objection to the sense of either of these points, I'll work on
updating the code-review document on the website and send it out for comment.
- Nelson
On Thu, Mar 24, 2011 at 05:42:50PM -0400, David A Benjamin wrote:
> I would personally like to see fewer complaints on style in our
> reviews. I think it is poor to suggest that our project has either a
> consistent style guide or consistent style in the codebase. While I
> would generally be in favor of improving style, this is a very
> subjective and iffy case. As long as it's at least consistent with
> surrounding code, I think we should let it slide. Or at least never
> hold off on signing off on the commit.
>
> The current situation is the style complaints are largely whatever
> the particular reviewer happened to prefer this week in the code.
> The only thing remotely consistent I've seen brought up is that
> kretchdoesn'tlikespacesandthisisbad so we have an unwritten rule to
> try to put them in we modify code. But even that isn't always
> followed; 82b734a72b592 removed a space in favor of making it more
> consistent with surrounding code.
>
> I think until we have a specific style guide, AND our codebase
> follows it everywhere, we should not be calling people out on all
> but the most egregious of faults. It's discouraging for a new
> contributor to get bogged down by stylistic rules that are neither
> consistent nor written. Especially when all they were doing is being
> consistent with nearly code. At the least, preface such comments
> with something like "Nit:" and don't hold off on signing off the
> change, so that signal can be separated from noise.
>
> As an anecdote, I am an occasional Chromium contributor. They are
> generally very anal about style, but that's okay, because it is very
> well specified in the style guide. Except I was writing a CL for
> some Mac code at some point, and it turns out the Mac reviewer I
> picked really hated royal we in comments. Unfortunately, I
> compulsively use it everywhere because of math background. :-) It
> was trivial to fix, but it was rather annoying as someone who was
> starting out with the project, as nowhere was this specified in the
> style guide, and other comments used it.
>
> David
>
> On Thu, 24 Mar 2011, Anders Kaseorg wrote:
>
> >On Wed, 23 Mar 2011, Nelson Elhage wrote:
> >>And so largely code review ends up just nitpicking on style and other
> >>local issues, that, while not irrelevant, also just don't matter all
> >>that much.
> >
> >BTW, at least when I review code, the style comments are produced as a
> >byproduct of reading and understanding the code well enough to check it
> >for more global issues. I don’t think there’s anything wrong with a
> >review that consists entirely of style nitpicking if, in fact, style is
> >the only problem.
> >
> >Anders
> >
