unity-dev team mailing list archive

["Stephen M. Webb" <stephen.webb@xxxxxxxxxxxxx>]

On 04/09/2013 05:14 AM, Sam Spilsbury wrote:

Thank you, Sam, for bringing this topic up.
> 
> [This] guideline should be followed when making such changes:
>  1. If making a substantive change (eg, a change in behaviour, fixing
> the logic or a bug) keep the formatting, stylistic and other minor
> changes restricted to the code section in which the application logic
> is being changed. Do not change formatting, indentation, variable
> declarations or perform any other refactoring in unrelated code
> sections in the same, or other files.
>  2. Merge proposals which change only indentation, formatting,
> variable declarations and perform other refactoring are acceptable and
> encouraged (so long as they improve the quality of the code), however,
> they must be marked as separate so that reviewers know what to look
> out for.

To summarise: one change per merge proposal.  Whitespace changes and functional changes are two separate changes.

I'd also like to add:  make your changes as small as possible.  If you have a large change to propose, see if you can
reduce it to several smaller incremental changes with dependent merge proposals for sequencing.  It may not be how you
develop the changes, but it's almost always possible and there are tools [1] that make this easier.  Smaller is better.

> Doing changes in this manner means that reviewers can get through both
> sets faster, because the type of cognitive load is different for each.
> In addition, it means that substantive changes aren't blocked on
> formatting changes which other reviewers might have suggestions on.

Better. Stronger. Faster.

Other large community projects I've worked on have a policy of rejecting violating changesets.  Those projects have a
reputation for quality and longevity.  We can seriously consider adopting such a stance.

[1] http://doc.bazaar.canonical.com/plugins/en/pipeline-plugin.html

-- 
Stephen M. Webb  <stephen.webb@xxxxxxxxxxxxx>