> likely provide merge pain Is there anyone that actually does merge commits w/adjustments to code for any non-trivial patches, or does everyone else have to "-s ours" with per-branch bespoke implementations and a --amend to the merge commit the way I have to? Not to pile on project frustrations with a thread; just genuinely curious.
If we have a consensus of preference for the K&R style, we could theoretically wait until immediately after accord merges then bulk update all our branches w/bracing adjustment. I think we'd be able to surgically do only that using intellij w/"Reformat Code" on a folder in the tree structure. If someone created a profile constrained down to only bracing style, I suspect we could hit it in one go. Now, the effect on git blame and potential loss of history w/out doing something painful like imerge and history / annotation preservation? less good. Maybe Jon has some ideas; he's done a lot of black magic with git. It would definitely have its costs, but it also wouldn't be a lot of toil if timed and executed surgically. On Thu, Jan 16, 2025, at 3:34 PM, Tolbert, Andy wrote: > > Isn't that what ant generate-idea-files does automatically? (that it sets > > code style like that) > > Unless i'm not doing something right (very possible), I don't think it does > this automatically with generate-idea-files. I've gotten in the habit of > updating my Checkstyle plugin to pull in .build/checkstyle.xml whenever I > regenerate my project. The checkstyle plugin is definitely very useful. > > It doesn't currently enforce any code style around braces since no rule is > currently being enforced in the checkstyle configuration. > > Thanks, > Andy > > On Thu, Jan 16, 2025 at 2:16 PM Š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 >>>>>>>>> <https://cassandra.apache.org/_/development/code_style.html> 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 >>>>>> >>>>
