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, borderline OCD hitting that shortcut every now and then just because I always want to look at the code which complies to the code style we have. Agreed! It looks like the formatter does a good job with this out of the box with generate-idea-files, I also don't think there is anything to improve on here as I think turning on auto-formatting should be an opt-in. Maybe for new projects doing that from the start this is fine, but for an established project I think it makes more sense to not hijack IDE configurations. I think a good contribution to the Code Style <https://cassandra.apache.org/_/development/code_style.html> guide could be to add a concise sentence or two in the "Format files for IDEs" providing some guidance of this option. Would be happy to contribute something if folks agree. Thanks, Andy On Sat, Jan 18, 2025 at 2:36 PM Blake Eggleston <beggles...@apple.com> wrote: > 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 quality of code they’re going to contribute to the project. > > It might also be useful to familiarize ourselves with various fallacies > that are common in arguments so we don’t inadvertently post a textbook > slippery slope argument. > > > On Jan 18, 2025, at 5:27 AM, Josh McKenzie <jmcken...@apache.org> wrote: > > 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 codebase, your brain is far more nimble than mine these > days. Extrapolating out from "if we formalize one thing, logically we're > going to formalize everything and then everything will change and then > nobody will know how to find anything anymore in our codebase" feels quite > melodramatic to me. > > The general impression I get from this thread is that more people would be > in favor of some more style formalization than not, but the people that are > against it are very strongly against it. Which is fine, a vote's a vote no > matter how strongly someone feels about something, and conversely a DISCUSS > thread like this is the right place to communicate those strong feelings > and perspectives. > > For me personally I'm not that bothered by the lack of things codified in > checkstyle, but neither would I be against us formalizing more of our style > guidelines into automated format-checking. As I mentioned earlier I'd be in > favor of us introducing this as a new optional checkstyle target people > could opt-into and, seeing how that goes over time, we discuss further > action or don't from there. > > On Fri, Jan 17, 2025, at 3:27 PM, Štefan Miklošovič wrote: > > 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? > > Because we should be consistent, right? Well, good luck with applying all > of that. If you did that, the resulting change would be so big that nobody > could navigate in the code base anymore. > > Developers are mentally visualising where the code is and how it looks. > 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_, you are used to that > and you expect that code to be there. > > If we reformat that too, then people would need to learn that stuff all > over again and it would slow them down greatly. > > On Fri, Jan 17, 2025 at 8:46 PM Jordan West <jw...@apache.org> wrote: > > 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 the vast majority of > the community does. > > Outside of “pride” is there a technical reason not to have an automatic > formatter? Have we seen them lead to bugs or egregious issues that waste > time? Right now my take (backed up anecdotally only) is we would waste more > time not having one (and comments to the effect of “the reviewer or > committer can do it” are evidence of that point). If folks want it to be > opt-in/out that’s a fine compromise although given the stance of most of > the community that’s chimed in here, not ideal imo. > > The curly braces I would love to see addresssed and I think doing it after > a major merge is smart. As a fork owner that would absolutely be the best > time as major upgrade merges are already super tough. But ultimately I am > +0 on it. I am +1 on whatever we decide going into an automatic formatter. > > Jordan > > On Fri, Jan 17, 2025 at 10:46 Jon Haddad <j...@rustyrazorblade.com> wrote: > > 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 codebases and > use auto formatting than require people to carefully write code that > loosely conforms to a style guide with artisanal spacing. > > I'm not sure who benefits from the manual work. > > Code is meant to be read, consistency is good, and makes it easier. > > > Jon > > > > On Fri, Jan 17, 2025 at 10:12 AM Benedict <bened...@apache.org> wrote: > > > 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? > > > On 17 Jan 2025, at 17:55, Cheng Wang via dev <dev@cassandra.apache.org> > wrote: > > > 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 <che...@netflix.com> wrote: > > 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 change the Version Control -> Commit and check Option "Reformat Code" to > ensure every time I commit it will automatically reformat the code. So as > you can probably see, it's a very manual and inconsistent process which > will cause more pains in the future (In my prior jobs I've seen 10+ code > styles in a single code base, so I can feel the pain.) I am a strong > advocate to enforce and reformat automatically at commit time (Most > projects do the same at Netflix). It might be a one-off cost but I think it > will save a lot of pain in the long run. > > On Fri, Jan 17, 2025 at 9:39 AM Maxim Muzafarov <mmu...@apache.org> wrote: > > 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 committing, and validating on CI > with the checkstyle that this hotkey was actually pressed? > When did this transition happen, when social communication became > better for engineers in pointing out the wrong codestyle than having > automation to avoid it? :-) > > Regardless of the codestyle, formatting the code with hotkeys and > validating it on the CI saves both the contributor and the reviewer > time in reading boring guides and writing code. So I would +1 for both > enforcing checkstyle lint (with braces on a new line), validating it > on CI, and at the same time fixing the IDE codestyle settings for code > formatting, alongside storing the settings in the project root, so > that everyone has the same config. > > On Fri, 17 Jan 2025 at 16:30, Caleb Rackliffe <calebrackli...@gmail.com> > wrote: > > > > 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: > >>> > >>> 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. > >>> 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. > >>> > >>> 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. > >>> Caleb pointed out we can do that trunk only. > >>> 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. > >>> > >>> 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: > >>> > >>> 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 > >>> > >>> > >>> > >>> > > >
