> I’m not letting anyone near my carefully crafted code with an automated > formatter.
Ha. Same. Let’s not get carried away. The main problem here seems to be overzealous code reviewers, and this can be solved with better reviewer onboarding / guide. The minor formatting nits can and should be fixed on commit by the committer, with a post-factum note to the (non-committer) author that this was done and a link to the code style. Reviewer should not be creating roundtrips for the author based on style nits alone. Fix it on commit, please, don’t waste everyone’s time. > On 17 Jan 2025, at 04:39, Abe Ratnofsky <a...@aber.io> wrote: > > If breaking blame is a concern, we can address that with the ignore-revs-file > flag in git and GitHub blame viewers: > https://docs.github.com/en/repositories/working-with-files/using-files/viewing-and-understanding-files#ignore-commits-in-the-blame-view > >> On Jan 16, 2025, at 18:11, Cheng Wang via dev <dev@cassandra.apache.org> >> wrote: >> >> >> I am personally in favor of more automatic code linting and enforcement than >> worrying about the code style. >> >> On Thu, Jan 16, 2025 at 1:56 PM Josh McKenzie <jmcken...@apache.org >> <mailto:jmcken...@apache.org>> wrote: >>>> 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 >>>> <mailto: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 >>>> <mailto: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 >>>>> <mailto: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 >>>>> <mailto: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 >>>>> <mailto: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 >>>>>>> <mailto: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 <mailto: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 >>>>>>>> <mailto: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 >>>>>>>> <mailto: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 <mailto: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 >>>>>>> >>>>> >>>> >>>
