On Jan 16, 2025, at 18:11, Cheng Wang via dev <dev@cassandra.apache.org> wrote:
I am personally in favor of more automatic code linting and enforcement than worrying about the code style. I merely remember Josh saying
I say a lot of things. :D And I reserve the right to change my mind in the face of better thought through arguments.
It could just be done in trunk, after any huge outstanding feature branches are merged, and eventually it would just be a memory.
I'm receptive to this. I strongly dislike the wasted vertical real-estate of our bracing style.
On Thu, Jan 16, 2025, at 4:28 PM, Štefan Miklošovič wrote:
oh well, I was wrong, they were all big bangs:
here we avoided star import everywhere and updated IDE configuration around that
here we sorted imports
I merely remember Josh saying (yeah, really :D but I can't find that) that it _should_ be rather gradual. But then we most probably just pulled the trigger anyway, I really dont remember.
All seasoned contributors already "got it" and there are no issues as far as I can tell.
The "wrong" braces are most often happening for one-off patches and in that situation it just does not make sense to explain that to people. In that case I sometimes fix that on commmit, yes.
For multi-branch patches, I am striving to not do anything with it. It really matters what it is. If I see one brace off ... whatever. But if it is a consistent pattern over the patch I tend to fix that all.
We were holding back other patches related to massive "code refactoring". If you remember, we had some new rules around imports. I think we somehow enforced that imports will be explicitly enumerated (no * imports) plus they are following the same order (how is that enforced, heh? This is also an IDEA thing, braces might be similar) but we have not refactored all codebase. We said that once the code style / enforcement is in place, then all these changes will be done naturally as new code is added or modified. That means that it will not be one "big bang" patch and it will rather happen gradually.
We have also contemplated refactoring javadocs etc. Maxim will know more.
likely provide merge pain
Is there anyone that actually does merge commits w/adjustments to code for any non-trivial patches, or does everyone else have to "-s ours" with per-branch bespoke implementations and a --amend to the merge commit the way I have to? Not to pile on project frustrations with a thread; just genuinely curious.
If we have a consensus of preference for the K&R style, we could theoretically wait until immediately after accord merges then bulk update all our branches w/bracing adjustment. I think we'd be able to surgically do only that using intellij w/"Reformat Code" on a folder in the tree structure. If someone created a profile constrained down to only bracing style, I suspect we could hit it in one go.
Now, the effect on git blame and potential loss of history w/out doing something painful like imerge and history / annotation preservation? less good. Maybe Jon has some ideas; he's done a lot of black magic with git.
It would definitely have its costs, but it also wouldn't be a lot of toil if timed and executed surgically.
On Thu, Jan 16, 2025, at 3:34 PM, Tolbert, Andy wrote:
> Isn't that what ant generate-idea-files does automatically? (that it sets code style like that)
Unless i'm not doing something right (very possible), I don't think it does this automatically with generate-idea-files. I've gotten in the habit of updating my Checkstyle plugin to pull in .build/checkstyle.xml whenever I regenerate my project. The checkstyle plugin is definitely very useful.
It doesn't currently enforce any code style around braces since no rule is currently being enforced in the checkstyle configuration.
Thanks,
Andy
_ The burden shouldn’t be on humans to place or check that every curly brace is on its own line._
Who is actually doing this? It is one shortcut in IDEA and it all places them correctly. I don't even know what the shortcut is, I never think about that consciously anymore.
Isn't that what ant generate-idea-files does automatically? (that it sets code style like that)
A committer can just format that upon merge, we do not need to stress about that during reviews.
IMO the more we can enforce the style guide programmatically the better. It was a big improvement when we got parts of it in IntelliJ. It saves time and reduces friction. The burden shouldn’t be on humans to place or check that every curly brace is on its own line. And if we say don’t check during review or automatically then why have a guide? Sure we can nitpick on the definition of the word “guide” here but I think we all mostly agree a uniform style (with some wiggle room for edge cases) is good for the project. Or again, why have the guide?
The challenge is getting these tools to do what we want can be a pain as folks who have explored it have outlined. So then I think it comes back to Josh’s question (to paraphrase) of “is it good enough as is”? And as an aside we might want to ask ourselves “why have we chosen a style guide that is hard to implement in these tools?”
If folks see some easy wins and want to volunteer time to make the changes I think we should encourage that.
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.
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.
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?
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.😁
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
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_."
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
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.
>
> 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
|
