Open Stack

On 2/23/2015 9:18 PM, Lana Brindley wrote:
> On 24/02/15 09:00, Nick Chase wrote:
>> Basically, as far as the ReviewGuidelines, we would add a new type of
>> change, making the three possibilities "Objective", "Subjective", and
>> "Cosmetic/Convention".  So for a (probably silly, but demonstrative)
>> example:
>>
>> -- You submit a change that adds a new section talking about how to add,
>> say, a new driver for Cinder, and everywhere you should have "OpenStack
>> Block Storage" you just say "cinder".  You a
>> -- I review your change, and I see that everything is fine, except for
>> those 17 instances of "cinder".
>> -- From here it can go one of three ways:
>> -- 1) I comment that I'm going to fix it (so you don't spend time on
>> it), then correct them and submit a patch, and +1.
>> -- 2) I mark them, and you fix them and resubmit.  I +1.
>> -- 3) I let you know that it needs to be fixed and:
>> -- -- 3a) you submit a bug and drop it into comments, then I +1 or
>> -- -- 3b) I submit a bug, then +1
>
> So, this isn't totally insane. From where I'm sitting though, I'd 
> vastly prefer to see 1 and 2 happen in 90%+ cases. I think we need to 
> be having a broader discussion about review rigour in general, though, 
> so we can better identify where a -1 isn't warranted, where a reviewer 
> should do a quick fix on someone else's patchset, or where a new bug 
> is a better way of fixing issues.

I think that's an excellent idea.

>
> I know when I started working on OpenStack docs, doing reviews was 
> quite daunting, mostly because I didn't have a good idea what level of 
> review rigour was expected. Here's some notes I wrote about that at 
> the time:
>
>> A note on review rigour: There are very few guidelines about what 
>> consists of a successful patch, but the general approach seems to be 
>> that if it's technically accurate and better than the existing 
>> content, then it should be approved. The main things to look for:
>> General spelling and grammar.
>> Technical accuracy. Where possible, test commands on your own VM to 
>> make sure they're accurate. Check any related bugs, and the Disqus 
>> comments on the doc site to see if there's anything else you might 
>> need to take into account.
>> The 'is it better than what we have already' test. Check the diff, or 
>> go look at the current document on the doc site, and determine if the 
>> changes are an improvement.
>> Provide corrections in-line (double click on the offending line in 
>> the diff viewer to write your suggested changes) for the author to 
>> fix if there's more than a couple of errors. If there's just one or 
>> two really minor changes (or in a situation where the writer is 
>> either ESL or could be otherwise unable to improve the doc 
>> themselves), consider checking out the patch and editing it quickly 
>> yourself. Be nice.
>
> Hopefully this can start a broader conversation about what is/isn't 
> expected from reviewers :)

These are great guidelines, Lana!  And I really like the idea of 
discussing expectations.

----  Nick