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