Stale MPs! Review discussions and Mailing Lists!

[Kevin DuBois]

On Mon, Apr 14, 2014 at 6:58 PM, Daniel van Vugt <
daniel.van.vugt at canonical.com> wrote:

> I was wondering how long it would take for someone else to raise this
> issue, not wanting to be the first to complain... ;)
>
> I think the simple best practice is: Check all reviews you are involved in
> every day or so. If you can't keep up with that then abstain or set WIP.
>

I think there are other things that can improve the situation too. My two
(three) cents:

1) I don't know what the distinction between 'needs fixings' vs
'disapproves' means. Both entail some responsibilities and communication
between the blocker and the author. It is sometimes unclear what it would
take to remove a blocking comment. A blocking comment that comes with a
clear route to unblock the objection is much more helpful than 'don't like
it!' This is particularly helpful with naming

2) We should use/cite the style guide more, and codify the conventions in
the code so its clear how to do certain things. As a code writer, I just
want to follow the conventions of the team. As a reviewer, I just want to
keep the code that I'm reviewing consistent with the code base / style
guide.

If someone chooses a different style than the style guide, that should get
a needs fixing.
If someone chooses a different style than a reviewer prefers (and the style
guide is silent/ambiguous), lets debate about style preferences as a team
so we can move forward on the minor points and not have to keep revisitng
the sore spots as blocking 'needs fixings'

3) We should be mindful of our public interfaces. Changing things in
include/ should get the same level of scrutiny that a change to mir's
client api should get. We are somewhat aware of this due to the pain it
causes downstream/in the silos, but as reviewers, we really should make
sure that the public interfaces are good interfaces; interfaces we'd like
to use as a 3rd party.

Cheers,
Kevin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/mir-devel/attachments/20140417/713e11fa/attachment.html>