Dear reviewers,

[Jesse Meek]

On 03/12/14 16:58, Andrew Wilkins wrote:
> On Wed, Dec 3, 2014 at 11:43 AM, Ian Booth <ian.booth at canonical.com 
> <mailto:ian.booth at canonical.com>> wrote:
>
>     On 03/12/14 13:34, Tim Penhey wrote:
>     > Hello there reviewers,
>     >
>     > I have a number of concerns around reviews that I need to say.
>     >
>     > Firstly, as an on call reviewer, you only need to look at the
>     reviews
>     > that have not yet been looked at.  If you ask for changes on a
>     branch asl
>     > a reviewer, you have a responsibility to respond to the
>     developer when
>     > they make said changes or they ask for clarification.
>     >
>
>     +1
>     I know sometimes I forget to go back the next day and look at
>     subsequent
>     changes, hence....
>
>
>     > As a developer it is your responsibility to get your branch landed.
>     > Don't just throw it over the review fence and think you are done.
>     >
>
>     ... sometimes the developer needs to poke the reviewer and remind
>     them to finish
>     the review :-)
>
>
> Agree with prodding for PTAL. It doesn't make sense to push the burden 
> onto everyone else.
>
> It seems reasonable to expect a branch with *no* reviews to receive 
> them quickly - within a day or two, unless there is a glut of 
> unreviewed branches.
> If you see a branch that hasn't been reviewed, please leave a comment 
> about *why* you're not reviewing it (e.g. the branch is too big, is 
> doing too much at once, etc.)
>
>     > Please be pragmatic when using the errors library. It doesn't
>     make sense
>     > to have it on absolutely every error return. It can be helpful,
>     but it
>     > isn't a requirement to be everywhere. As a developer, consider
>     > annotating errors when it makes sense and tracing where
>     appropriate. As
>     > a reviewer, please don't expect it everywhere.
>     >
>
>     +1
>     Let's be pragmatic and consider the situation. eg I would not
>     expect trace to be
>     required when returning an error at a layer boundary, but deep
>     down inside some
>     complex function, yes. With annotate, it may not be needed it an
>     immediate
>     caller annotates the error for example. So please use good
>     judgement rather than
>     a blanket insistence that trace and annotate be used everywhere.
>
>
> Yes. Otherwise we end up with "could not do x: could not do x: could 
> not do x: could not do x: x could not be done because reasons".
>
> Add enough information so that we can diagnose where errors came from 
> to diagnose the root cause. Err on the side of adding more information 
> than less, but be thoughtful about it: too much information will 
> obscure things.
>
> Generally: if you're making a suggestion in a review, base that 
> suggestion on the outcome and not on some arbitrary rule.

I'm guilty of peppering reviews recommending to use the errors package. 
I'll tone this back.

>
>     > With respect to string literals: if it is used once, inline is fine;
>     > twice is borderline; more than twice and there should be a
>     (hopefully
>     > documented) defined constant.  The constant should have a meaningful
>     > name, not obscure.
>     >
>
>     +100 for so many reasons
>
>     --
>     Juju-dev mailing list
>     Juju-dev at lists.ubuntu.com <mailto:Juju-dev at lists.ubuntu.com>
>     Modify settings or unsubscribe at:
>     https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20141204/4ef0157d/attachment.html>