[3012] in BarnOwl Developers
Re: Thoughts on our current code review policy
daemon@ATHENA.MIT.EDU (David A Benjamin)
Thu Mar 24 17:42:53 2011
Date: Thu, 24 Mar 2011 17:42:50 -0400 (EDT)
From: David A Benjamin <davidben@MIT.EDU>
To: Anders Kaseorg <andersk@mit.edu>
cc: Nelson Elhage <nelhage@nelhage.com>, barnowl-dev@mit.edu
In-Reply-To: <alpine.DEB.2.02.1103240309380.4468@dr-wily.mit.edu>
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
---1257051904-1618696675-1301002970=:25741
Content-Type: TEXT/PLAIN; charset=UTF-8; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
I would personally like to see fewer complaints on style in our reviews. I=
=20
think it is poor to suggest that our project has either a consistent style=
=20
guide or consistent style in the codebase. While I would generally be in=20
favor of improving style, this is a very subjective and iffy case. As long=
=20
as it's at least consistent with surrounding code, I think we should let=20
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=20
particular reviewer happened to prefer this week in the code. The only=20
thing remotely consistent I've seen brought up is that=20
kretchdoesn'tlikespacesandthisisbad so we have an unwritten rule to try to=
=20
put them in we modify code. But even that isn't always followed;=20
82b734a72b592 removed a space in favor of making it more consistent with=20
surrounding code.
I think until we have a specific style guide, AND our codebase follows it=
=20
everywhere, we should not be calling people out on all but the most=20
egregious of faults. It's discouraging for a new contributor to get bogged=
=20
down by stylistic rules that are neither consistent nor written.=20
Especially when all they were doing is being consistent with nearly code.=
=20
At the least, preface such comments with something like "Nit:" and don't=20
hold off on signing off the change, so that signal can be separated from=20
noise.
As an anecdote, I am an occasional Chromium contributor. They are=20
generally very anal about style, but that's okay, because it is very well=
=20
specified in the style guide. Except I was writing a CL for some Mac code=
=20
at some point, and it turns out the Mac reviewer I picked really hated=20
royal we in comments. Unfortunately, I compulsively use it everywhere=20
because of math background. :-) It was trivial to fix, but it was rather=20
annoying as someone who was starting out with the project, as nowhere was=
=20
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=E2=80=99t think there=E2=80=99s anything w=
rong with a
> review that consists entirely of style nitpicking if, in fact, style is
> the only problem.
>
> Anders
>
---1257051904-1618696675-1301002970=:25741--
