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 -- Mailing list: https://launchpad.net/~unity-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~unity-dev More help : https://help.launchpad.net/ListHelp
