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, Jan 16, 2025 at 2:20 PM Jordan West <jw...@apache.org> wrote: > > 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 better / easier. > > Jordan > > On Thu, Jan 16, 2025 at 12:17 Štefan Miklošovič <smikloso...@apache.org> > wrote: >> >> _ 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 generate-idea-files does automatically? (that it sets >> code style like that) >> >> A committer can just format that upon merge, we do not need to stress about >> that during reviews. >> >> On Thu, Jan 16, 2025 at 8:44 PM Jordan West <jw...@apache.org> wrote: >>> >>> 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 >>> review or automatically then why have a guide? Sure we can nitpick on the >>> definition of the word “guide” here but I think we all mostly agree a >>> uniform style (with some wiggle room for edge cases) is good for the >>> project. Or again, why have the guide? >>> >>> The challenge is getting these tools to do what we want can be a pain as >>> folks who have explored it have outlined. So then I think it comes back to >>> Josh’s question (to paraphrase) of “is it good enough as is”? And as an >>> aside we might want to ask ourselves “why have we chosen a style guide that >>> is hard to implement in these tools?” >>> >>> If folks see some easy wins and want to volunteer time to make the changes >>> I think we should encourage that. >>> >>> On Thu, Jan 16, 2025 at 07:35 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 >>>> >>>> >>>>
