unity-dev team mailing list archive

[Sam Spilsbury <smspillaz@xxxxxxxxx>]

Hey all,

I've seen a few merge proposals (at least on compiz) starting to be
submitted which include both minor refactoring and formatting fixes as
well and substantive changes. I think the former changes are usually
always acceptable within reason if they improve the quality of the
code, but I've found that they've been blocking and distracting
substantive bits of changes where they are mixed.

Here's a guideline which I posted on a recent merge proposal. I'm
wondering if this should be considered a general policy for unity
projects to adopt?

Indentation fixes and code cleanups are useful and appreciated.
However, they tend to just confuse things when mixed with substantive
changes. Especially when the reviewer isn't immediately familiar with
the section of code that's being edited.

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

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.

Best,

Sam

--
Sam Spilsbury