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 >
