You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Donal Evans <do...@vmware.com> on 2021/05/27 17:22:21 UTC
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
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Patrick Johnson <jp...@vmware.com>.
+1. Also really like Naba's idea of incorporating this into spotless.
On May 27, 2021, at 10:46 AM, Raymond Ingles <ri...@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" <do...@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
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Raymond Ingles <ri...@vmware.com>.
+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" <do...@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
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Donal Evans <do...@vmware.com>.
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 <kl...@apache.org>
Sent: Friday, May 28, 2021 3:19 PM
To: dev@geode.apache.org <de...@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 <da...@vmware.com> wrote:
> +1 thanks for working on this
> ________________________________
> From: Donal Evans <do...@vmware.com>
> Sent: Thursday, May 27, 2021 10:22 AM
> To: dev@geode.apache.org <de...@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%7Cdoevans%40vmware.com%7C6d413e51e7814e9999e308d92226b180%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637578371864383775%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=4An8MCpjP60dPIt%2BXbkmplHh6F8pc%2F0KdpTPO2KnvNI%3D&reserved=0
> [2]
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523&data=04%7C01%7Cdoevans%40vmware.com%7C6d413e51e7814e9999e308d92226b180%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637578371864383775%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=pufQ770JEdbJ%2FfWgT0rqQmrLJyFqQQjFc98Q%2FkYnHq4%3D&reserved=0
>
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Kirk Lund <kl...@apache.org>.
+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 <da...@vmware.com> wrote:
> +1 thanks for working on this
> ________________________________
> From: Donal Evans <do...@vmware.com>
> Sent: Thursday, May 27, 2021 10:22 AM
> To: dev@geode.apache.org <de...@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%7Cdarrel%40vmware.com%7C55c1f47da50548d3fa5708d92134034c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329548326057%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BVvzQAdQ24ZuoOdoMrkGNJShmTkep4BGFduSAQu5H6o%3D&reserved=0
> [2]
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523&data=04%7C01%7Cdarrel%40vmware.com%7C55c1f47da50548d3fa5708d92134034c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329548326057%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=gzta%2FwrhapOzp0DKcMEn1IR5XsNboWuMlUJwUwOrcGc%3D&reserved=0
>
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Darrel Schneider <da...@vmware.com>.
+1 thanks for working on this
________________________________
From: Donal Evans <do...@vmware.com>
Sent: Thursday, May 27, 2021 10:22 AM
To: dev@geode.apache.org <de...@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%7Cdarrel%40vmware.com%7C55c1f47da50548d3fa5708d92134034c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329548326057%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BVvzQAdQ24ZuoOdoMrkGNJShmTkep4BGFduSAQu5H6o%3D&reserved=0
[2] https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523&data=04%7C01%7Cdarrel%40vmware.com%7C55c1f47da50548d3fa5708d92134034c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329548326057%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=gzta%2FwrhapOzp0DKcMEn1IR5XsNboWuMlUJwUwOrcGc%3D&reserved=0
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Jacob Barrett <ja...@vmware.com>.
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 <do...@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
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Nabarun Nag <nn...@vmware.com>.
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 <do...@vmware.com>
Sent: Thursday, May 27, 2021 10:22:21 AM
To: dev@geode.apache.org <de...@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%7Cnnag%40vmware.com%7Cc82e3e06a3fb4021675e08d921340678%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329595666744%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=pfi%2FBR%2FTNzlQM%2BUbo3Bwze%2Fg%2FOPbpkrjvIPprH5vVU4%3D&reserved=0
[2] https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523&data=04%7C01%7Cnnag%40vmware.com%7Cc82e3e06a3fb4021675e08d921340678%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329595666744%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=idYV4eKaAC3gEpu9hr0x%2BLXUtbwHEcLJC3ZIwOZlLHo%3D&reserved=0
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Udo Kohlmeyer <ud...@vmware.com>.
+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 <do...@vmware.com>
Date: Friday, May 28, 2021 at 6:36 AM
To: dev@geode.apache.org <de...@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 <ag...@vmware.com>
Sent: Thursday, May 27, 2021 1:20 PM
To: dev@geode.apache.org <de...@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" <de...@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 <do...@vmware.com>
Date: Thursday, May 27, 2021 at 10:22 AM
To: dev@geode.apache.org <de...@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
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Donal Evans <do...@vmware.com>.
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 <ag...@vmware.com>
Sent: Thursday, May 27, 2021 1:20 PM
To: dev@geode.apache.org <de...@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" <de...@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%7Cdoevans%40vmware.com%7C9c71bf203b4d474515ea08d9214cdbb1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577436255763312%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=i91hCIEYZGStlSLSFZWgmvrj4rv1vlvv7bPJ2ChlHIM%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 <do...@vmware.com>
Date: Thursday, May 27, 2021 at 10:22 AM
To: dev@geode.apache.org <de...@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%7Cdoevans%40vmware.com%7C9c71bf203b4d474515ea08d9214cdbb1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577436255773270%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Twrc0TddPBTxgAeCBecbyMMSAW42X1SfyECBZwm9oeY%3D&reserved=0
[2] https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523&data=04%7C01%7Cdoevans%40vmware.com%7C9c71bf203b4d474515ea08d9214cdbb1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577436255773270%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5ivxrQAyGmGQ9zCRKOZuK0%2F8bHVKTeJqXVhVLxzVpFM%3D&reserved=0
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Anilkumar Gingade <ag...@vmware.com>.
+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" <de...@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%7Cagingade%40vmware.com%7C0e6c904d94d2441787a908d921381910%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577347090159625%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=dYh4JefNS%2FspVSOLmOHh6zi9jiq3sS9Cs0GWMrn18sI%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 <do...@vmware.com>
Date: Thursday, May 27, 2021 at 10:22 AM
To: dev@geode.apache.org <de...@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%2F6523&data=04%7C01%7Cagingade%40vmware.com%7C0e6c904d94d2441787a908d921381910%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577347090159625%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BD2XFs4hc2qcQS9dl9k7wd3EVkqoD5Cm44tDqrzWhn0%3D&reserved=0
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Dale Emery <de...@vmware.com>.
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 <do...@vmware.com>
Date: Thursday, May 27, 2021 at 10:22 AM
To: dev@geode.apache.org <de...@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%2F6523&data=04%7C01%7Cdemery%40vmware.com%7Ca2cc7d62b84049fff7bc08d921340535%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329575017080%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ZmyQo5GL6fqtQ7twsI1mDK%2BT0K6%2B6CAiO0Nba5ZkFAw%3D&reserved=0
Re: Cleaning up the codebase - use curly braces everywhere
Posted by Sarah Abbey <sa...@vmware.com>.
This is great! Thank you for doing this, Donal!