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