Re: Checkstyle as style contract for Cassandra

Fri, 17 Jan 2025 07:31:01 -0800

If we can bake it into the two IDEA settings that control class and method opening brace placement, WFM

On Jan 17, 2025, at 8:28 AM, Štefan Miklošovič <smikloso...@apache.org> wrote:


I am sorry if I read this incorrectly but the vibe I am getting is that we are going to rework that. 

On Fri, Jan 17, 2025 at 3:22 PM Štefan Miklošovič <smikloso...@apache.org> wrote:
Are really new-line braces so much pain? It is interesting to see this, really.  What are the main problems with that? You can just format that by shortcuts in IDEA and I suggested that we might explore how to make it the part of generate-idea-files. What are we trying to solve by reformatting 2k+ files to have braces elsewhere?

On Fri, Jan 17, 2025 at 3:05 PM Josh McKenzie <jmcken...@apache.org> wrote:
As is tradition, this thread has split off into a few topics; fwiw I take this as a very positive sign as it means we all care a lot about the project and what we work on, and it's a sign we should maybe talk more frequently about discrete topics instead of remembering adjacent topics when something like this comes up and piling on. ;)

Let me try and round them up and snapshot any indications of consensus:
  1. Should we automate linting / formatting? Strong no from ay / bes, some loose opinions in favor of it. Maybe a compromise would be having a checkstyle target that'd include formatting people could optionally run locally but not formally integrating it into CI; make it opt-in.
  2. Are we happy with our bracing style, and would it be too painful to change it now? Seems like, in general, we range from -1 to -0 for all but one or two outliers who are +1.
    1. Abe pointed out (in a forked thread in my email client /sad) that we can use a --ignore-revs-file option in git to switch bracing style in one go and keep our history.
    2. Caleb pointed out we can do that trunk only.
    3. Mick seconded raised concerns about forks absorbing pain. It'd be a post-accord consideration so at least mainline rebase pain would be minimized, and if we kept it to trunk-only that'd probably be fine.
  3. Should we put together a review guideline for the project? Worth considering for us as a project; Benedict indicated receptivity to us having one based on the google one here.
So, Bernardo: hopefully the general "vibes" of what you were shooting for on this thread initially are answered in terms of us covering our surface area of the status quo. Shall we break out into 3 [DISCUSS] threads for each of the above 3 topics and put this thread to rest?

On Fri, Jan 17, 2025, at 6:36 AM, Benedict wrote:

I would support adopting a review guide based on this one.

On 16 Jan 2025, at 15:36, Josh McKenzie <jmcken...@apache.org> wrote:

Perhaps a “Review Guide” is what we need to make sure we keep review primarily focused on the core contribution, and to help avoid folk getting bogged down in style sniping.
I recall reading through / offering this guide in the past as a starting point for an org I was managing at the time: https://google.github.io/eng-practices/review/reviewer/

Been years; might be worth it to have a skim through that and see if it could serve as a reasonable starting point for us if someone has the inclination.

On Thu, Jan 16, 2025, at 9:17 AM, Benedict wrote:

I can imagine that it might cause some frustrating review interactions people would like to avoid, but for solving that I’d prefer we take a more social approach. 

Review shouldn’t spend much time on minor style points, and these should normally be framed as suggestions. Obviously newer contributors may need pointing to the style guide as something to familiarise themselves with, but it shouldn’t readily be invoked as a “thou shalt do this” tool. 

Perhaps a “Review Guide” is what we need to make sure we keep review primarily focused on the core contribution, and to help avoid folk getting bogged down in style sniping.


On 16 Jan 2025, at 14:08, Josh McKenzie <jmcken...@apache.org> wrote:

Right now our codebase is pretty consistent, especially for not having a linter enforcing this kind of thing. Are we trying to solve for codebase consistency, education of new contributors, both? Neither?

If just solving for consistency I'd argue we're good. If educating new contributors, the Code Style guide seems pretty thorough to me? https://cassandra.apache.org/_/development/code_style.html

All of which is to say - it feels like the status quo is fine here for me. i.e. it's not clear to me what problem we're trying to solve w/a change here.

On Wed, Jan 15, 2025, at 9:58 PM, guo Maxwell wrote:
I agree with you for all these two points.

I think you should open a ticket to solve this if you want to add a rule to checkstyle, as I know there are many old codes that do not comply with this rule.
For point 2, this really feels like personal preference, but I'd probably listen to the reviewer's opinion.😁

Tolbert, Andy <x...@andrewtolbert.com> 于2025年1月16日周四 08:47写道：
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 and had some thoughts on how to make this work but hit a bit of a brick wall:


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



Reply via email to