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%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 <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%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
