RFR: 8332313: Update code review guidelines [v2]

[Kevin Rushforth]

On Thu, 16 May 2024 08:48:14 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> I agree with the concern, but I still think it's much better to encourage developers to do formatting in a separate issue (or not at all) with all the required administration, than to sneak in a formatting change in a PR that has nothing to do with formatting. I prefer the noise to be completely isolated, so that it can be ignored easily, rather than being distracted by it in a PR.
>
> I think this conflicts a bit with the previous statement ("don't worry too much about import order"). In my experience, when merging complex changes, import order changes are often the cause of conflicts. We also found that with many different committer preferences, the whole import block could change from one commit to the next, which will lead to a conflict if such a file is part of a backport.
> 
> We solved this by simply mandating a single order for imports (alphabetical, no gaps, no star imports, followed by separate block for static imports with the same rules) and made the build fail when it was violated.  The number of conflicts went down dramatically.  This does require a one time import fix of the code base, which will also be a source of conflicts, but at least it will be in the usual area (imports) and should solve the problem once and for all.

I removed the bit encouraging reformatting PRs (but left the "don't reformat existing code..." bit.

As for the import order, this came up recently in Andy's PRs to remove unnecessary imports. I'll file a follow-on issue to evaluate whether we want to change our current policy.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605067813