+1. Also really like Naba's idea of incorporating this into spotless. On May 27, 2021, at 10:46 AM, Raymond Ingles <ring...@vmware.com<mailto:ring...@vmware.com>> wrote:
+1 Sounds like a good idea. Since always using curly braces is part of the style guide, fixing this makes sense. The only possible additional step for large-but-automatically-generated-and-specifically-targeted PRs like this is running the test suite with code coverage, making sure the updated code paths are actually exercised by the tests. I think this particular change is low-risk, but that might be a good check as humans aren't all that great at checking 3300 instances of anything. And if there *are* gaps, it'll point to some good tests to write. On 5/27/21, 1:22 PM, "Donal Evans" <doev...@vmware.com<mailto: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://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FGEODE%2FCode%2BStyle%2BGuide&data=04%7C01%7Cjpatrick%40vmware.com%7C1a3fbc078ac94bca8b0608d921375879%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577343862113013%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=wGDiVSNZl%2FzRDPxG01ta60jwsHh%2BzLvRlsdQFAy0a1s%3D&reserved=0 [2] https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523&data=04%7C01%7Cjpatrick%40vmware.com%7C1a3fbc078ac94bca8b0608d921375879%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577343862113013%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Ae%2B9ssxeIZ5jdXqVLZXuzBgNy%2FxU4OAVlSwg8LuPyt8%3D&reserved=0
