On Wed, Dec 3, 2014 at 11:43 AM, Ian Booth <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 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 :-) 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. > 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 > 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
