Andy, I have checked if ant generate-idea-files sets up the formatter correctly and it seems it still does the job.
When I removed .idea dir and opened e.g. cassandra-5.0 branch in IDEA, then the code style / formatter was not applied. But as soon as you execute "ant generate-idea-files" on a clean project (without .idea dir, I think it will be actually replaced when found but anyway), then I see in the code style example window that braces are put on the new lines (Settings -> Editor -> Code Style -> Java -> Wrapping and Braces - I see braces are on new lines out of the box). So I think we do not need to do anything. It is all configured properly already. People just need to hit the formatter (ctrl + shift + l). You also correctly pointed out that you can format automatically on saving so you don't even need to think about it. I reformat code very often, borderline OCD hitting that shortcut every now and then just because I always want to look at the code which complies to the code style we have. On Thu, Jan 16, 2025 at 9:46 PM Tolbert, Andy <x...@andrewtolbert.com> wrote: > Hi Stefan, > > > It is somewhere in Settings -> Editor -> Code Style -> Java -> Wrapping > and Braces and there you can fiddle with it endlessly. Maybe putting that > into some xml as part of generate-idea-files would do it? > > Ah yes, good point, that evidentally does work well. Code > Reformat > File, does appear to put braces on the next line (except for Lambdas which > is what we want), so that actually works quite well. > > I also see that there is also an option in IntelliJ you can set to > Reformat code only in changed lines. Navigate to Tools > Action On Save > > Reformat Code and set "Changed lines". I'll be making use of that from > here on :) > > Thanks, > Andy > > On Thu, Jan 16, 2025 at 2:42 PM Štefan Miklošovič <smikloso...@apache.org> > wrote: > >> Couldn't say it better. >> >> My position is that I do not mind too much where the braces are (new line >> or not). I do mind that it is / would be inconsistent. At this point, I >> think we keep putting them on new lines just because the majority of that >> already is and nobody wants to break the consistency. >> >> On the other hand, If it was my choice solely, I got used to the braces >> on new lines over time and I just find it better / visually more pleasing. >> I understand that this is a highly personal matter though. >> >> Andy, >> >> It is somewhere in Settings -> Editor -> Code Style -> Java -> Wrapping >> and Braces and there you can fiddle with it endlessly. Maybe putting that >> into some xml as part of generate-idea-files would do it? >> >> I honestly do not have a clue how that was configured in my case. I have >> just "set it up just once and live with it for a decade" mindset. >> >> On Thu, Jan 16, 2025 at 9:30 PM Brandon Williams <dri...@gmail.com> >> wrote: >> >>> 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 >>> >>>> >>> >>>> >>> >>>> >>> >>
