Re: Cleaning up the codebase - use curly braces everywhere
Given the positive response to this idea, I'm going to mark the PR as ready for review and contact the code owners required to get it merged. Thanks for all the feedback, and hopefully this can be the first step towards a concerted clean-up effort. Donal From: Kirk Lund Sent: Friday, May 28, 2021 3:19 PM To: dev@geode.apache.org Subject: Re: Cleaning up the codebase - use curly braces everywhere +1 Also, I think if the entire work is done by IntelliJ then just mention that in the PR description and I'm happy to add approval without stepping through every line. I'd probably just pick a dozen random ones to check and then approve it. On Thu, May 27, 2021 at 6:36 PM Darrel Schneider wrote: > +1 thanks for working on this > > From: Donal Evans > Sent: Thursday, May 27, 2021 10:22 AM > To: 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%2BGuidedata=04%7C01%7Cdoevans%40vmware.com%7C6d413e51e7814ee308d92226b180%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637578371864383775%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=4An8MCpjP60dPIt%2BXbkmplHh6F8pc%2F0KdpTPO2KnvNI%3Dreserved=0 > [2] > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523data=04%7C01%7Cdoevans%40vmware.com%7C6d413e51e7814ee308d92226b180%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637578371864383775%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=pufQ770JEdbJ%2FfWgT0rqQmrLJyFqQQjFc98Q%2FkYnHq4%3Dreserved=0 >
Re: Cleaning up the codebase - use curly braces everywhere
+1 Also, I think if the entire work is done by IntelliJ then just mention that in the PR description and I'm happy to add approval without stepping through every line. I'd probably just pick a dozen random ones to check and then approve it. On Thu, May 27, 2021 at 6:36 PM Darrel Schneider wrote: > +1 thanks for working on this > > From: Donal Evans > Sent: Thursday, May 27, 2021 10:22 AM > To: 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%2BGuidedata=04%7C01%7Cdarrel%40vmware.com%7C55c1f47da50548d3fa5708d92134034c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329548326057%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=BVvzQAdQ24ZuoOdoMrkGNJShmTkep4BGFduSAQu5H6o%3Dreserved=0 > [2] > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523data=04%7C01%7Cdarrel%40vmware.com%7C55c1f47da50548d3fa5708d92134034c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329548326057%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=gzta%2FwrhapOzp0DKcMEn1IR5XsNboWuMlUJwUwOrcGc%3Dreserved=0 >
Re: Cleaning up the codebase - use curly braces everywhere
+1 thanks for working on this From: Donal Evans Sent: Thursday, May 27, 2021 10:22 AM To: 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%2BGuidedata=04%7C01%7Cdarrel%40vmware.com%7C55c1f47da50548d3fa5708d92134034c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329548326057%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=BVvzQAdQ24ZuoOdoMrkGNJShmTkep4BGFduSAQu5H6o%3Dreserved=0 [2] https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523data=04%7C01%7Cdarrel%40vmware.com%7C55c1f47da50548d3fa5708d92134034c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329548326057%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=gzta%2FwrhapOzp0DKcMEn1IR5XsNboWuMlUJwUwOrcGc%3Dreserved=0
Re: Cleaning up the codebase - use curly braces everywhere
+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
Date: Friday, May 28, 2021 at 6:36 AM
To: 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
Sent: Thursday, May 27, 2021 1:20 PM
To: 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" 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.htmldata=04%7C01%7Cudo%40vmware.com%7Cee349edf6b654bb61ebb08d9214f14d4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577445839008597%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=GkQeG05zMbkLN9d5Z%2BWgHXd2BfOyHdc07Cj1hpDmVQk%3Dreserved=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
Date: Thursday, May 27, 2021 at 10:22 AM
To: 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%2BGuidedata=04%7C01%7Cudo%40vmware.com%7Cee349edf6b654bb61ebb08d9214f14d4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577445839008597%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=i790IJeUu9cjFar8cT%2Fb2yu3j%2Br%2FNYghKPMLp76B94Y%3Dreserved=0
[2]
https://nam
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
Sent: Thursday, May 27, 2021 1:20 PM
To: 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" 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.htmldata=04%7C01%7Cdoevans%40vmware.com%7C9c71bf203b4d474515ea08d9214cdbb1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577436255763312%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=i91hCIEYZGStlSLSFZWgmvrj4rv1vlvv7bPJ2ChlHIM%3Dreserved=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
Date: Thursday, May 27, 2021 at 10:22 AM
To: 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%2BGuidedata=04%7C01%7Cdoevans%40vmware.com%7C9c71bf203b4d474515ea08d9214cdbb1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577436255773270%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=Twrc0TddPBTxgAeCBecbyMMSAW42X1SfyECBZwm9oeY%3Dreserved=0
[2]
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523data=04%7C01%7Cdoevans%40vmware.com%7C9c71bf203b4d474515ea08d9214cdbb1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577436255773270%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=5ivxrQAyGmGQ9zCRKOZuK0%2F8bHVKTeJqXVhVLxzVpFM%3Dreserved=0
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" 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.htmldata=04%7C01%7Cagingade%40vmware.com%7C0e6c904d94d2441787a908d921381910%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577347090159625%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=dYh4JefNS%2FspVSOLmOHh6zi9jiq3sS9Cs0GWMrn18sI%3Dreserved=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
Date: Thursday, May 27, 2021 at 10:22 AM
To: 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://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%2F6523data=04%7C01%7Cagingade%40vmware.com%7C0e6c904d94d2441787a908d921381910%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577347090159625%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=BD2XFs4hc2qcQS9dl9k7wd3EVkqoD5Cm44tDqrzWhn0%3Dreserved=0
Re: Cleaning up the codebase - use curly braces everywhere
We might also use IntelliJ to enforce any guidelines that we want to enforce. You can run inspections on the command line: https://www.jetbrains.com/help/idea/command-line-code-inspector.html 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 Date: Thursday, May 27, 2021 at 10:22 AM To: 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://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%2F6523data=04%7C01%7Cdemery%40vmware.com%7Ca2cc7d62b84049fff7bc08d921340535%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329575017080%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=ZmyQo5GL6fqtQ7twsI1mDK%2BT0K6%2B6CAiO0Nba5ZkFAw%3Dreserved=0
Re: Cleaning up the codebase - use curly braces everywhere
+1. Also really like Naba's idea of incorporating this into spotless. On May 27, 2021, at 10:46 AM, Raymond Ingles 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" 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%2BGuidedata=04%7C01%7Cjpatrick%40vmware.com%7C1a3fbc078ac94bca8b0608d921375879%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577343862113013%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=wGDiVSNZl%2FzRDPxG01ta60jwsHh%2BzLvRlsdQFAy0a1s%3Dreserved=0 [2] https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523data=04%7C01%7Cjpatrick%40vmware.com%7C1a3fbc078ac94bca8b0608d921375879%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577343862113013%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=Ae%2B9ssxeIZ5jdXqVLZXuzBgNy%2FxU4OAVlSwg8LuPyt8%3Dreserved=0
Re: Cleaning up the codebase - use curly braces everywhere
+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" 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%2BGuidedata=04%7C01%7Cringles%40vmware.com%7C3391c856ca6f4715dc0308d921340687%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329609790016%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=6AYrCHhgS0jPJiCcfAZZ2pA2hmS%2B2E2heQDYlUyo0Nw%3Dreserved=0 [2] https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523data=04%7C01%7Cringles%40vmware.com%7C3391c856ca6f4715dc0308d921340687%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329609790016%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=AMD7tgnhnc0HSFFQCNHd2e4hpfoETrPC8sH30SOafaQ%3Dreserved=0
Re: Cleaning up the codebase - use curly braces everywhere
This is great clean up idea. I think you should go for it. Regarding the check, I think we can try to incorporate it in our spotless script to fix it if we miss brackets in blocks . Regards Nabarun Get Outlook for iOS<https://aka.ms/o0ukef> From: Donal Evans Sent: Thursday, May 27, 2021 10:22:21 AM To: 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%2BGuidedata=04%7C01%7Cnnag%40vmware.com%7Cc82e3e06a3fb4021675e08d921340678%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329595666744%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=pfi%2FBR%2FTNzlQM%2BUbo3Bwze%2Fg%2FOPbpkrjvIPprH5vVU4%3Dreserved=0 [2] https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523data=04%7C01%7Cnnag%40vmware.com%7Cc82e3e06a3fb4021675e08d921340678%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329595666744%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=idYV4eKaAC3gEpu9hr0x%2BLXUtbwHEcLJC3ZIwOZlLHo%3Dreserved=0
Re: Cleaning up the codebase - use curly braces everywhere
This is great! Thank you for doing this, Donal!
Re: Cleaning up the codebase - use curly braces everywhere
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 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%2F6523data=04%7C01%7Cjabarrett%40vmware.com%7C118d498a6faf4c8f0f5a08d92134027c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329531722126%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=OmbmslBmbIqnlq8ZW9Xr6yHCORu1eWLz%2FDR4umU6eFI%3Dreserved=0
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://cwiki.apache.org/confluence/display/GEODE/Code+Style+Guide [2] https://github.com/apache/geode/pull/6523
