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