Re: Checkstyle as style contract for Cassandra

I was coming to start the threads suggested by Josh just to find out that he had already done that. Thanks a lot Josh! I waited a little bit too long to give people the opportunity to chime in as my questions seemed to have sparked a lively conversation :-) For visibility, those threads are: [

Re: Checkstyle as style contract for Cassandra

We are already coming forward with how contributors should go around PRs here (1). That is the default PR template which is posted to every PR when not changed by the contributor before creating it. I can't think of a better place to show this information to contributors. ant generate-idea-files i

Re: Checkstyle as style contract for Cassandra

Hi Stefan, > So I think we do not need to do anything. It is all configured properly already. People just need to hit the formatter (ctrl + shift + l). > You also correctly pointed out that you can format automatically on saving so you don't even need to think about it. I reformat code very often

Re: Checkstyle as style contract for Cassandra

Let’s also keep in mind that this is a discussion about the pros and cons of automatic formatting. Everyone wants what’s best for the project, although they may have different views about how to get there. One person thinking we should use a tool does not make them lazy, nor does it affect the

Re: Checkstyle as style contract for Cassandra

Andy, I have checked if ant generate-idea-files sets up the formatter correctly and it seems it still does the job. When I removed .idea dir and opened e.g. cassandra-5.0 branch in IDEA, then the code style / formatter was not applied. But as soon as you execute "ant generate-idea-files" on a cle

Re: Checkstyle as style contract for Cassandra

> You have a map in your brain of how to read the code base. You know where to > find these things. And they are _exactly there_, If you've memorized the specific placement of where things are within discrete files Stefan and you're able to keep up with all the other diffs and churn in our codeb

Re: Checkstyle as style contract for Cassandra

If we extend automatic formatting beyond just braces, where does that end? Are we going to place rules where static variables are? Static code blocks, constructors, methods and private methods, setters, getters, toString, equals ... Is the order of these things going to be directed as well? Becaus

Re: Checkstyle as style contract for Cassandra

People who don’t take pride in the little things tend to have little pride in the big things. Pride is what produces better code that is more readable. I have experience with codebases where automatic formatters reign, and my experience is that it does not improve readability, it just encourages pe

Re: Checkstyle as style contract for Cassandra

I too had the same experience as Jon (not with Go but with a project that had a pre-commit linter and another that did it in IntelliJ — much nicer than the pre-commit formatter). If we have a guide it should be as automatable as possible. I feel strongly about that. From the responses I also think

Re: Checkstyle as style contract for Cassandra

Fwiw, I used to have the same opinion that I don't need or want an auto formatter, than I tried Go where you don't have a choice. I realized how pointless it was to do it manually. Every Go codebase looks the same, and it's awesome. I'd rather see C* be consistent with the majority of Java codeba

Re: Checkstyle as style contract for Cassandra

I’m amazed at the number of people who take no pride in their formatting decisions, and would abdicate responsibility to an automatic formatter.But, that’s fine and you should all feel welcome to do that. Just maybe don’t try to force it on those of us who do take pride in our formatting decisions?

Re: Checkstyle as style contract for Cassandra

And in an ideal world, we should never worry about the code style. Pointing out code style issues in PRs is a waste of time for both contributors and reviewers, imo. On Fri, Jan 17, 2025 at 9:52 AM Cheng Wang wrote: > Just share my personal experience as a new contributor to Cassandra. > It's ab

Re: Checkstyle as style contract for Cassandra

Just share my personal experience as a new contributor to Cassandra. It's about the new-line braces. My muscle memory is the same line braces, so I need to set the Inteliij code style to have the Brace Placement option to Next Line, and do a Reformat Code for the files I changed. Also, I need to ch

Re: Checkstyle as style contract for Cassandra

As a personal feeling from reading the thread: Am I right in thinking that we are forcing new contributors to read long contribution guides (in addition to spending time writing them) in favour of just pressing Option+Cmd+L (or other hotkeys in the IDE they like) to format the code before committi

Re: Checkstyle as style contract for Cassandra

If we can bake it into the two IDEA settings that control class and method opening brace placement, WFMOn Jan 17, 2025, at 8:28 AM, Štefan Miklošovič 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 M

Re: Checkstyle as style contract for Cassandra

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č 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 fo

Re: Checkstyle as style contract for Cassandra

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+

Re: Checkstyle as style contract for Cassandra

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 somet

Re: Checkstyle as style contract for Cassandra

> I’m not letting anyone near my carefully crafted code with an automated > formatter. Ha. Same. Let’s not get carried away. The main problem here seems to be overzealous code reviewers, and this can be solved with better reviewer onboarding / guide. The minor formatting nits can and should be

Re: Checkstyle as style contract for Cassandra

I would support adopting a review guide based on this one.On 16 Jan 2025, at 15:36, Josh McKenzie 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 thr

Re: Checkstyle as style contract for Cassandra

On Thu, 16 Jan 2025 at 22:06, Caleb Rackliffe wrote: > … > There are forks out there, of course, and they would be affected, too. > This. Let's make sure to check thoroughly, this could really hurt people (for what is a trivial annoyance). Personally, braces on newlines doesn't bother me. I

Re: Checkstyle as style contract for Cassandra

I’m not letting anyone near my carefully crafted code with an automated formatter.On 17 Jan 2025, at 04:40, Abe Ratnofsky wrote:If breaking blame is a concern, we can address that with the ignore-revs-file flag in git and GitHub blame viewers: https://docs.github.com/en/repositories/working-with-

Re: Checkstyle as style contract for Cassandra

If breaking blame is a concern, we can address that with the ignore-revs-file flag in git and GitHub blame viewers: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-and-understanding-files#ignore-commits-in-the-blame-viewOn Jan 16, 2025, at 18:11, Cheng Wang via dev w

Re: Checkstyle as style contract for Cassandra

I am personally in favor of more automatic code linting and enforcement than worrying about the code style. On Thu, Jan 16, 2025 at 1:56 PM Josh McKenzie wrote: > 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

Re: Checkstyle as style contract for Cassandra

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

Re: Checkstyle as style contract for Cassandra

oh well, I was wrong, they were all big bangs: here we avoided star import everywhere and updated IDE configuration around that https://github.com/apache/cassandra/pull/2041 here we sorted imports https://github.com/apache/cassandra/pull/2108 https://lists.apache.org/thread/d3s3ghkb81f7mbb6t09

Re: Checkstyle as style contract for Cassandra

> It would definitely have its costs, but it also wouldn't be a lot of toil if timed and executed surgically. It could just be done in trunk, after any huge outstanding feature branches are merged, and eventually it would just be a memory. There are forks out there, of course, and they would be af

Re: Checkstyle as style contract for Cassandra

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

Re: Checkstyle as style contract for Cassandra

> 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

Re: Checkstyle as style contract for Cassandra

Hi Stefan, > It is somewhere in Settings -> Editor -> Code Style -> Java -> Wrapping and Braces and there you can fiddle with it endlessly. Maybe putting that into some xml as part of generate-idea-files would do it? Ah yes, good point, that evidentally does work well. Code > Reformat File, does

Re: Checkstyle as style contract for Cassandra

Couldn't say it better. My position is that I do not mind too much where the braces are (new line or not). I do mind that it is / would be inconsistent. At this point, I think we keep putting them on new lines just because the majority of that already is and nobody wants to break the consistency.

Re: Checkstyle as style contract for Cassandra

> 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/ch

Re: Checkstyle as style contract for Cassandra

I think we inherited brace-on-newline, nobody has ever liked it (stockholm syndrome aside.) As I recall we decided against changing it in the past because it would be a huge patch and likely provide merge pain for a long time, unless we do it in all the branches. Kind Regards, Brandon On Thu, Ja

Re: Checkstyle as style contract for Cassandra

I think we are in strong agreement Stefan. It’s more convenient because we took the time to make it automatic and not require human effort. Yet it’s still imperfect as Bernardo pointed out. And as Caleb pointed out, had we taken a more convenential Java project approach it would’ve been even bett

Re: Checkstyle as style contract for Cassandra

_ 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 gene

Re: Checkstyle as style contract for Cassandra

> “why have we chosen a style guide that is hard to implement in these tools?” This has bothered me since I first looked at this code 14 years ago. How hard would it be to just stop doing the newline braces and be like nearly every other large Java project in existence? On Thu, Jan 16, 2025 at 1:

Re: Checkstyle as style contract for Cassandra

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

Re: Checkstyle as style contract for Cassandra

> 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

Re: Checkstyle as style contract for Cassandra

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

Re: Checkstyle as style contract for Cassandra

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 C

Re: Checkstyle as style contract for Cassandra

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'

Re: Checkstyle as style contract for Cassandra

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(

Re: Checkstyle as style contract for Cassandra

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: https://issues.apache.org/jira/browse/

Re: Checkstyle as style contract for Cassandra

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

Re: Checkstyle as style contract for Cassandra

I agree with you on the second one that it seems more like a preference. The first one is indeed a rule and I appreciate it that you want to add a rule and fix any places where we already fail it. I think the more we automate such kind of things the better. Thanks again for bringing it in. Best

Checkstyle as style contract for Cassandra

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 i