> I merely remember Josh saying I say a lot of things. :D And I reserve the right to change my mind in the face of better thought through arguments.
> It could just be done in trunk, after any huge outstanding feature branches > are merged, and eventually it would just be a memory. I'm receptive to this. I strongly dislike the wasted vertical real-estate of our bracing style. On Thu, Jan 16, 2025, at 4:28 PM, Štefan Miklošovič wrote: > oh well, I was wrong, they were all big bangs: > > here we avoided star import everywhere and updated IDE configuration around > that > > https://github.com/apache/cassandra/pull/2041 > > here we sorted imports > > https://github.com/apache/cassandra/pull/2108 > > https://lists.apache.org/thread/d3s3ghkb81f7mbb6t09gy6t2nvl94nyy > > I merely remember Josh saying (yeah, really :D but I can't find that) that it > _should_ be rather gradual. But then we most probably just pulled the trigger > anyway, I really dont remember. > > On Thu, Jan 16, 2025 at 10:04 PM Štefan Miklošovič <smikloso...@apache.org> > wrote: >> All seasoned contributors already "got it" and there are no issues as far as >> I can tell. >> >> The "wrong" braces are most often happening for one-off patches and in that >> situation it just does not make sense to explain that to people. In that >> case I sometimes fix that on commmit, yes. >> >> For multi-branch patches, I am striving to not do anything with it. It >> really matters what it is. If I see one brace off ... whatever. But if it is >> a consistent pattern over the patch I tend to fix that all. >> >> We were holding back other patches related to massive "code refactoring". If >> you remember, we had some new rules around imports. I think we somehow >> enforced that imports will be explicitly enumerated (no * imports) plus they >> are following the same order (how is that enforced, heh? This is also an >> IDEA thing, braces might be similar) but we have not refactored all >> codebase. We said that once the code style / enforcement is in place, then >> all these changes will be done naturally as new code is added or modified. >> That means that it will not be one "big bang" patch and it will rather >> happen gradually. >> >> We have also contemplated refactoring javadocs etc. Maxim will know more. >> >> On Thu, Jan 16, 2025 at 9:52 PM Josh McKenzie <jmcken...@apache.org> wrote: >>> __ >>>> 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 >>>>>>>>> >>>>>>> >>>
