+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> 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%7Cringles%40vmware.com%7C3391c856ca6f4715dc0308d921340687%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329609790016%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=6AYrCHhgS0jPJiCcfAZZ2pA2hmS%2B2E2heQDYlUyo0Nw%3D&reserved=0
[2]
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523&data=04%7C01%7Cringles%40vmware.com%7C3391c856ca6f4715dc0308d921340687%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329609790016%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=AMD7tgnhnc0HSFFQCNHd2e4hpfoETrPC8sH30SOafaQ%3D&reserved=0
