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