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