On Thu, 25 Apr 2024 12:58:36 GMT, robert engels <d...@openjdk.org> wrote:
>> We have been using these newer constructs whenever a new test gets added, >> but we don't update all tests in one go for such changes. If/when old tests >> are updated for some bug fix relevant to that test, depending on the >> complexity we either decide to let them stay as-is or update them to use >> these newer constructs. > > Personally, I think that is not the correct choice. I think consistency is > more important - especially in a code base this large. > > But I’ll do whatever you want. This is getting a bit frustrating. The PR > review is 1% content and 99% personal nits. > > This is certainly a different approach than Google uses. To clarify the above. The standard should be - does this PR improve the code base. After which, if someone wishes to create a refactoring PR that addresses other issues - go for it. But don’t drag out someone’s already considerable effort in diagnosing and fixing the problem to satisfy other goals. Clearly there is a lot of test code that could use cleanup - but this is not the way to go about doing that - holding up important fixes as an example. Most developers are going to look to existing code when implementing changes - if you have a “bunch of stuff in your head” you’d really like it to adhere to it is going to be a frustrating process and people won’t bother. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579479267
