Reading back https://issues.apache.org/jira/browse/CASSANDRA-19276 a bit
more, I think I *was* able to make checkstyle bend to the "Code Style"
definition by ignoring lambda tokens. It's just that there were a lot of
"violations" which defined a method on one line:
public int getActiveTaskCount() { return 0; }
public long getCompletedTaskCount() { return 0; }
public int getPendingTaskCount() { return 0; }
public int getCorePoolSize() { return 0; }
public int getMaximumPoolSize() { return 0; }
I felt that this code was perfectly readable and wouldn't be right to
change. This is what I wanted to make checkstyle consider acceptable.
I think it would be really nice if checkstyle would fail for the more
obvious case we want to avoid that comes up in reviews or sometimes slips
into the codebase if not caught by a reviewer, e.g.:
if {
//...
}
Thanks,
Andy
On Wed, Jan 15, 2025 at 6:21 PM Tolbert, Andy <x...@andrewtolbert.com> wrote:
> Hi Bernardo,
>
> Thanks for bringing this up!
>
> Last year I was looking into enforcing curly braces as defined in Code
> Style <https://cassandra.apache.org/_/development/code_style.html> and
> had some thoughts on how to make this work but hit a bit of a brick wall:
>
> https://issues.apache.org/jira/browse/CASSANDRA-19276
>
> I don't think there is an easy way as is to enforce this with checkstyle
> currently:
>
> "{ and } are placed on a new line except when empty or opening a
> multi-line lambda expression. Braces may be elided to a depth of one if the
> condition or loop guards a single expression."
>
> Without making changes to checkstyle itself (e.g.:
> https://github.com/checkstyle/checkstyle/issues/12226).
>
> I think if we were to add a new rule around brackets and newlines, we
> would ideally try to make it match the Code style definition as its
> declared, and hopefully it would not be too require touching a lot of files
> (which maybe the case unfortunately).
>
> Thanks,
> Andy
>
> On Wed, Jan 15, 2025 at 6:10 PM Benedict <bened...@apache.org> wrote:
>
>> Even something as simple as the curly brace rule has sensible exceptions.
>> I’m pretty hard -1 on letting a linter make all our editing decisions.
>> Formatting is a contextual choice about how to best represent information
>> to the reader, and we should not abdicate responsibility. The style guide
>> is exactly that, a guide and that helps us navigate editing choices, and it
>> can be evolved or refined via discussion and experimentation.
>>
>> For example, the second clause in your quote (re: lambdas) came about
>> only because we could break the restrictions of the first clause and
>> demonstrate an improvement to readability.
>>
>> If this is a pain point during review, either some people are too eager
>> to point to the code style guide, or perhaps your IDE defaults need
>> updating. This shouldn’t cause lots of traffic.
>>
>> People should try not to overly nitpick formatting, though of course a
>> balance is to be struck between contributors’ expression of their code and
>> that code sitting neatly in its context in the codebase.
>>
>> > On 15 Jan 2025, at 23:50, Bernardo Botella <
>> conta...@bernardobotella.com> wrote:
>> >
>> > Hi everyone!
>> >
>> > I wanted to raise a question about code style for the project. I've
>> been receiving some feedback on PRs about the need to:
>> > - Have curly braces start on a new line
>> > - Remove curly braces if the condition or loop has only one expression
>> >
>> > Taking a look at the official Code Style stated in the web, I read that:
>> > "{ and } are placed on a new line except when empty or opening a
>> multi-line lambda expression. Braces may be elided to a depth of one if the
>> condition or loop guards a single expression."
>> >
>> > Which addresses the first type of comments I mentioned (curly braces
>> starting in a new line), but leaves open the second type of comments
>> (remove not needed curly braces).
>> >
>> > But, when looking at the checkstyle.xml, I don't see any rule enforcing
>> any of those two types of comments.
>> >
>> > I believe checkstyle.xml should be our contract, so I'm proposing here:
>> >
>> > For "curly braces starting in a new line" rule, add something like what
>> we already have on Sidecar and Analytics projects:
>> > <module name="LeftCurly">
>> > <!-- Checks for placement of the left curly brace ('{'). -->
>> > <property name="option" value="nl"/>
>> > ...
>> > </module>
>> >
>> > That way, we can fail fast and not worry about those comments on PRs.
>> This of course may be painful, as we probably will have to fix a bunch of
>> wrongly placed brackets all over the place.
>> >
>> > If there are no concerns here, I'll be more than happy to bite the
>> bullet and add a patch for this.
>> >
>> >
>> >
>> > For "remove not needed curly braces", I understand that it tends to be
>> the preference on the code, so we either modify the documentation and add a
>> rule for that on the checkstyle.xml, or we are fine with that style and
>> there is no need to remove them on patches.
>> >
>> > I wanted to hear the thoughts on the community for this one. My
>> preference is to always use brackets, but that's just a preference, so it's
>> perfectly fine not to enforce it and leave the documentation as is.
>> >
>> > Thanks everyone!
>> > Bernardo
>>
>
