On Fri, 28 Feb 2025 21:14:54 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> I just copied the style that was being used, but can make any changes >> desired of course (at the expense of increasing the diff and amount of code >> to review). > > I know it's a general policy not to do unrelated changes or reformatting, but > I think in this case > a) it's a test and > b) you already touched it > so might as well. I can do this, but realize that the test is large, and there's comments like this in multiple places, also places I didn't touch. So, I can fix it here locally, resulting in this method standing out from the rest, or I can do this everywhere causing this PR to have a lot of non-changes that reviewers will need to be aware of and ignore. Normally I would do this by marking a commit as a "refactor" commit, in which functionality changes are prohibited, and so reviewers only need to look for changes that jump out as not just a minor refactor / rename / move. However, in the JDK repositories this isn't so easily done, and such commits are not retained (it is squashed together with functional commits), and so to keep them separated you must make a new ticket, new PR, and get new buy-in to get it included. I'm really trying to avoid to make a commit too large to review and drown out the functional changes with cosmetic changes, even though every time I touch code like `CssStyleHelper` it itches to remove redundant checks or give fields/variables more descriptive names. Even the changes I did in Region I was already questioning if this wouldn't be too much (there is more that I would have liked to change there, but I refrained as I fear this will just make it too hard to review). I face a similar dilemma with HBox/VBox modifications... do I do the same modifications twice (for the sake of reviews) or do I introduce an `AbstractBox`, remove all duplicate code, and then apply the modifications there, ensuring that it will be almost impossible to review and seeing what I actually changed... What we really need IMHO is a way to include refactors with functional changes. In my day job, we do this by having a specific commit order; we have one or a couple of commits that do minor cleanups or refactors -- no functionality is changed in these commits, leading to quick and easy reviews of those changes. Then the final commit (we force push to keep commits clean and focused during reviews -- the review tooling we use handles this without any problem) contains **only** the functional changes which, thanks to the earlier refactors, are often tiny changes that just change some parameters or add a function or two -- no noise, and reviewers can focus purely on functionality. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1976060714
