Thanks you!! I believe these targeted whole source tree changes like this are great. As long as it isn’t a mix of hand rolled and automated changes I think a reviewer can trust that the if heart of the change is correct there is no reason to review all 3000 lines changed. I think it gets harder when its a mix of changes, like apply all automated cleanup, in one PR or a mix of hand corrected and automated.
-Jake > On May 27, 2021, at 10:22 AM, Donal Evans <doev...@vmware.com> wrote: > > Hi Geode dev, > > I've recently been looking at ways to improve code quality in small ways > throughout the codebase, and as a starting point, I thought it would be good > to make it so that we're consistently using curly braces for control flow > statements everywhere, since this is something that's specifically called out > in the Geode Code Style Guide wiki page[1] as one of the "more important > points" of our code style. > > IntelliJ has a "Run inspection by name..." feature that makes it possible to > identify all places where curly braces aren't used for control flow > statements, (which showed over 3300 occurrences in the codebase) and also > allows them to be automatically inserted, making the fix relatively trivial. > Since this PR will touch 640 files, I wanted to make sure to first check that > this is something even worth doing, and, if there's agreement that it is, to > give reviewers context on what the changes are, the motivation for them, and > how they were made, to help with the review process. > > The draft PR I have up[2] currently has no failing tests and can be marked as > ready to review if there aren't any objections, and once it is, I'll try to > coordinate with codeowners to get the minimal number of approvals required > for a merge (it looks like only 6-7 reviewers are needed, though I'm sure > that almost every code owner will be tagged as reviewers given the number of > files touched). > > If this idea is a success, I think it would be good to have a discussion > about other low-hanging code improvements we could make using static analysis > (unnecessary casts, unused variables, duplicate conditions etc.), and, once a > particular inspection has been "fixed," possibly consider adding a check for > it as part of the PR pre-checkin to make sure it's not reintroduced. All > thoughts and feedback are very welcome. > > Donal > > [1] https://cwiki.apache.org/confluence/display/GEODE/Code+Style+Guide > [2] > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523&data=04%7C01%7Cjabarrett%40vmware.com%7C118d498a6faf4c8f0f5a08d92134027c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329531722126%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=OmbmslBmbIqnlq8ZW9Xr6yHCORu1eWLz%2FDR4umU6eFI%3D&reserved=0
