I am personally in favor of more automatic code linting and enforcement than worrying about the code style.
On Thu, Jan 16, 2025 at 1:56 PM Josh McKenzie <jmcken...@apache.org> wrote: > I merely remember Josh saying > > I say a lot of things. :D And I reserve the right to change my mind in the > face of better thought through arguments. > > It could just be done in trunk, after any huge outstanding feature > branches are merged, and eventually it would just be a memory. > > I'm receptive to this. I strongly dislike the wasted vertical real-estate > of our bracing style. > > On Thu, Jan 16, 2025, at 4:28 PM, Štefan Miklošovič wrote: > > oh well, I was wrong, they were all big bangs: > > here we avoided star import everywhere and updated IDE configuration > around that > > https://github.com/apache/cassandra/pull/2041 > > here we sorted imports > > https://github.com/apache/cassandra/pull/2108 > > https://lists.apache.org/thread/d3s3ghkb81f7mbb6t09gy6t2nvl94nyy > > I merely remember Josh saying (yeah, really :D but I can't find that) that > it _should_ be rather gradual. But then we most probably just pulled the > trigger anyway, I really dont remember. > > On Thu, Jan 16, 2025 at 10:04 PM Štefan Miklošovič <smikloso...@apache.org> > wrote: > > All seasoned contributors already "got it" and there are no issues as far > as I can tell. > > The "wrong" braces are most often happening for one-off patches and in > that situation it just does not make sense to explain that to people. In > that case I sometimes fix that on commmit, yes. > > For multi-branch patches, I am striving to not do anything with it. It > really matters what it is. If I see one brace off ... whatever. But if it > is a consistent pattern over the patch I tend to fix that all. > > We were holding back other patches related to massive "code refactoring". > If you remember, we had some new rules around imports. I think we somehow > enforced that imports will be explicitly enumerated (no * imports) plus > they are following the same order (how is that enforced, heh? This is also > an IDEA thing, braces might be similar) but we have not refactored all > codebase. We said that once the code style / enforcement is in place, then > all these changes will be done naturally as new code is added or modified. > That means that it will not be one "big bang" patch and it will rather > happen gradually. > > We have also contemplated refactoring javadocs etc. Maxim will know more. > > On Thu, Jan 16, 2025 at 9:52 PM Josh McKenzie <jmcken...@apache.org> > wrote: > > > likely provide merge pain > > Is there anyone that actually does merge commits w/adjustments to code for > any non-trivial patches, or does everyone else have to "-s ours" with > per-branch bespoke implementations and a --amend to the merge commit the > way I have to? Not to pile on project frustrations with a thread; just > genuinely curious. > > If we have a consensus of preference for the K&R style, we could > theoretically wait until immediately after accord merges then bulk update > all our branches w/bracing adjustment. I think we'd be able to surgically > do only that using intellij w/"Reformat Code" on a folder in the tree > structure. If someone created a profile constrained down to only bracing > style, I suspect we could hit it in one go. > > Now, the effect on git blame and potential loss of history w/out doing > something painful like imerge and history / annotation preservation? less > good. Maybe Jon has some ideas; he's done a lot of black magic with git. > > It would definitely have its costs, but it also wouldn't be a lot of toil > if timed and executed surgically. > > On Thu, Jan 16, 2025, at 3:34 PM, Tolbert, Andy wrote: > > > Isn't that what ant generate-idea-files does automatically? (that it > sets code style like that) > > Unless i'm not doing something right (very possible), I don't think it > does this automatically with generate-idea-files. I've gotten in the habit > of updating my Checkstyle plugin to pull in .build/checkstyle.xml whenever > I regenerate my project. The checkstyle plugin is definitely very useful. > > It doesn't currently enforce any code style around braces since no rule is > currently being enforced in the checkstyle configuration. > > Thanks, > Andy > > On Thu, Jan 16, 2025 at 2:16 PM Š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 <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 > > > > > >
