On Fri, 28 Feb 2025 22:09:19 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> 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. I think the main concern with the reformatting/tangentially related changes coming from the maintainers is the general pain associated with conflicts created when merging and backporting. In this case, I think we might be ok, specifically in the tests. I would rather see the cleaner and more maintainable code, even at the small additional expense. Up to you. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1976103486
