+1, I trust the work that you’ve done @Donal. I think splitting this out by package or any other metric we could come up with, it an exercise in futility.
So let’s get this merged in, set up a spotless rule to make sure we don’t introduce more and move on… --Udo From: Donal Evans <doev...@vmware.com> Date: Friday, May 28, 2021 at 6:36 AM To: dev@geode.apache.org <dev@geode.apache.org> Subject: Re: Cleaning up the codebase - use curly braces everywhere Regarding doing the changes at package level, there would still be a huge amount to review for certain code owners, and some code owners would get tagged to review multiple of the PRs, all of which would consist of nearly identical changes repeated over and over. For more complex change sets, breaking them up to make them more manageable makes sense, but for something like this, where it's an automatically applied process to do one very specific thing, expecting reviewers to look at every single line changed is excessive, I think. I believe a similar situation occurred when spotless was first applied to the project; an automatic process was applied, and reviewers trusted that it had been applied correctly (maybe checking that nothing was obviously wrong in a few files) but didn't manually check every single changed line. ________________________________ From: Anilkumar Gingade <aging...@vmware.com> Sent: Thursday, May 27, 2021 1:20 PM To: dev@geode.apache.org <dev@geode.apache.org> Subject: Re: Cleaning up the codebase - use curly braces everywhere +1 Instead of big merge; can this be done at package level; just a thought. -Anil. On 5/27/21, 10:51 AM, "Dale Emery" <dem...@vmware.com> wrote: We might also use IntelliJ to enforce any guidelines that we want to enforce. You can run inspections on the command line: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.jetbrains.com%2Fhelp%2Fidea%2Fcommand-line-code-inspector.html&data=04%7C01%7Cudo%40vmware.com%7Cee349edf6b654bb61ebb08d9214f14d4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577445839008597%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GkQeG05zMbkLN9d5Z%2BWgHXd2BfOyHdc07Cj1hpDmVQk%3D&reserved=0 An advantage of using IntelliJ inspections is that we can provide an inspection profile that treats violations as errors. Then developers can use this profile while editing to spot violations immediately as they’re introduced. A disadvantage is that this somewhat couples Geode to a particular IDE. Dale From: Donal Evans <doev...@vmware.com> Date: Thursday, May 27, 2021 at 10:22 AM To: dev@geode.apache.org <dev@geode.apache.org> Subject: Cleaning up the codebase - use curly braces everywhere 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://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FGEODE%2FCode%2BStyle%2BGuide&data=04%7C01%7Cudo%40vmware.com%7Cee349edf6b654bb61ebb08d9214f14d4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577445839008597%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=i790IJeUu9cjFar8cT%2Fb2yu3j%2Br%2FNYghKPMLp76B94Y%3D&reserved=0 [2] https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523&data=04%7C01%7Cudo%40vmware.com%7Cee349edf6b654bb61ebb08d9214f14d4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577445839008597%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=uyMytaHvJJ3FqvNvEKACGxTc1dhQyx%2B5KcKXOYoG4FQ%3D&reserved=0