I am sorry if I read this incorrectly but the vibe I am getting is that we are going to rework that.
On Fri, Jan 17, 2025 at 3:22 PM Štefan Miklošovič <smikloso...@apache.org> wrote: > Are really new-line braces so much pain? It is interesting to see this, > really. What are the main problems with that? You can just format that by > shortcuts in IDEA and I suggested that we might explore how to make it the > part of generate-idea-files. What are we trying to solve by reformatting > 2k+ files to have braces elsewhere? > > On Fri, Jan 17, 2025 at 3:05 PM Josh McKenzie <jmcken...@apache.org> > wrote: > >> As is tradition, this thread has split off into a few topics; fwiw I take >> this as a very positive sign as it means we all care a lot about the >> project and what we work on, and it's a sign we should maybe talk more >> frequently about discrete topics instead of remembering adjacent topics >> when something like this comes up and piling on. ;) >> >> Let me try and round them up and snapshot any indications of consensus: >> >> 1. *Should we automate linting / formatting?* Strong no from ay / >> bes, some loose opinions in favor of it. Maybe a compromise would be >> having >> a checkstyle target that'd include formatting people could optionally run >> locally but not formally integrating it into CI; make it opt-in. >> 2. *Are we happy with our bracing style, and would it be too painful >> to change it now?* Seems like, in general, we range from -1 to -0 for >> all but one or two outliers who are +1. >> 1. Abe pointed out (in a forked thread in my email client /sad) that >> we can use a --ignore-revs-file option in git to switch bracing style >> in >> one go and keep our history. >> 2. Caleb pointed out we can do that trunk only. >> 3. Mick seconded raised concerns about forks absorbing pain. It'd >> be a post-accord consideration so at least mainline rebase pain would >> be >> minimized, and if we kept it to trunk-only that'd probably be fine. >> 3. *Should we put together a review guideline for the project?* >> Worth considering for us as a project; Benedict indicated receptivity to >> us >> having one based on the google one here >> <https://google.github.io/eng-practices/review/reviewer/>. >> >> So, Bernardo: hopefully the general "vibes" of what you were shooting for >> on this thread initially are answered in terms of us covering our surface >> area of the status quo. Shall we break out into 3 [DISCUSS] threads for >> each of the above 3 topics and put this thread to rest? >> >> On Fri, Jan 17, 2025, at 6:36 AM, Benedict wrote: >> >> >> I would support adopting a review guide based on this one. >> >> On 16 Jan 2025, at 15:36, 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 <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 >> >> >> >> >>
