Dear reviewers,

[Ian Booth]

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 as
> 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 :-)

> 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.


> 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