On 03/12/14 16:58, Andrew Wilkins wrote:
On Wed, Dec 3, 2014 at 11:43 AM, Ian Booth <ian.bo...@canonical.com
<mailto:ian.bo...@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@lists.ubuntu.com <mailto:Juju-dev@lists.ubuntu.com>
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
