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 >> >>>> >> >>>> >> >>>> >> >
