couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nick North <nort...@gmail.com>
Subject Re: we need more reviews
Date Mon, 20 Jan 2014 14:30:27 GMT
On 20 January 2014 12:26, Dale Harvey <dale@arandomurl.com> wrote:

> I fully agree, its something I mentioned at the couchdb conf in vancouver,
> a review first system encourages contributions and has multiple benefits
>
>  * At least 2 people look at the code, less likely to push silly mistakes
>  * Can codify and practice review rules
>  * Its much easier to view the current activity in the project
>  * Can bring up points of discussion before its too late
>
> But I think the most important thing is that it removes the burden of
> responsibility from the committer to the project as a whole, also hugely
> beneficial is that it makes it particularly obvious when a patch has reach
> a stalemate and forces someone to make the call.
>
> For reference on PouchDB every committer is trusted to push code, nobody
> (including myself) pushes their own code to master, it goes in the PR queue
> and gets a +1 from any other committer (who will usually push it), thats
> essentially the same process we use at mozilla and coming to think of it
> any other project I have worked on, any commiter has the ability to -1 a
> patch at which point they give a reason and usually some solution gets
> agreed on
>

I like this system, with one small quibble about responsibility. I don't
think it should be seen as removing the burden of responsibility from the
committer to the project as a whole. It becomes easy then for everyone,
including the committer, to think someone else will find bugs, and no-one
puts in enough effort. I'd say it is still primarily the responsibility of
the committer to ensure that code is error free, but that at least one
person who knows that area of the code base should sign off on it. Some
spreading of responsibility is good, but too much can actually lead to a
decline in quality.


Nick

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message