-
Notifications
You must be signed in to change notification settings - Fork 503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Annotation wrapping before explicit constructor (ktlint_official) #2138
Comments
and
So, indeed the code looks quite different from before and it might need time to get used to it. Qualifiers like 'ugly', 'hard', 'better' and 'worse' are subjective and always related to what you are used to see. Also, I am still getting used to this new format.
This example, I have been doubting about. In some other context, the Kotlin style guide suggest two possibilties: Option 1:
or Option 2:
I have chosen the first option. Due to the rule that removes blank lines at the start of a class the result becomes:
Allowing a blank line in at start of class above would not be consistent with that rule. I am not inclined to change the formatting at this moment. I will leave the issue open for a while to collect other opinions. |
Also coming from a project that heavily uses Taking the original example: class Foo @Inject constructor(
private val someDependency: SomeDependency,
private val anotherDependency: AnotherDependency,
) : Bar {
//...
} In terms of token saliency when reading, I would argue Looking at the post-formatted example, I would argue that class Foo
@Inject
constructor(
private val someDependency: SomeDependency,
private val anotherDependency: AnotherDependency,
) : Bar {
//...
} Finally, this approach also means the whole class contents are shifted over by an additional indent, which feels a little wasteful. In our codebase, the I would suggest maybe enforcing the wrapping maybe only if more than one annotation is present. |
I like your style of reasoning. Despite that, there are other arguments not to allow to make an exception for the case of a single annotation. From a viewpoint of consistency, the rules which are applied on functions, are also applied on constructors. I know that constructors and functions are different beasts. But as with cars, they have both four wheels and some doors. The case of the
I do agree that the identation feels awkward. But I would wrap, and keep IntelliJ IDEA formatting, the result would be:
In this way the most important thing |
It's true that "ugly" is subjective, but my main point is that it is "hard to read", or in other words, "less readable", and that is not entirely subjective. "Better" and "worse" are also not entirely subjective in my opinion, as there are patterns that have proven to be more or less readable regardless of what people are used to.
Agreed! Thanks @lyallcooper for articulating this.
I understand the desire for consistency, but it is really a secondary concern. The primary concern is readability. Consistency generally improves readability, which is what makes consistency worthwhile. However, if something is done just to improve consistency and it actually reduces readability, then it defeats the purpose. A machine will never be able to make the best decision for human readability in 100% of cases. There are always going to be some cases where a human will have to make a judgment call. The Kotlin guide says as much in the way it is worded. I previously quoted: "A single annotation without arguments may be placed on the same line as the corresponding declaration." Here it says "may", denoting that it is an option for the human programmer to decide, not a strict rule. I am suggesting that this be made an option in harmony with the Kotlin official guidance, not that it be enforced to be on the same line in that case.
Generally, yes, constructors are similar to functions. However, I could easily argue that Kotlin primary constructors are very different and have a much stronger link to the class definition, but I do not have to, because even in the case of a regular function, the Kotlin guide says that a single annotation without arguments can be placed on the same line as the function declaration, even offering this example: @Test fun foo() { /*...*/ } Further, the Android guide also states, "When only a single annotation without arguments is present, it may be placed on the same line as the declaration," and offers this example: @Test fun selectAll() {
// …
} Obviously, there's a reason these guides agree and both saw fit to include this as an explicit option. There are definitely cases where it is most readable to put the single annotation without arguments on the same line, and my
Honestly, of all the different indentation examples where the annotation is on a separate line, I think this one is the most readable. However, I think every such example given here is deficient in some way. I don't think we can clearly say of any of them, "This is the best way to indent things in 100% of cases," so I don't think it makes sense to force it upon everyone. But this issue is really secondary to me, because I can avoid it altogether if I can put |
Let's agree to disagree on this topic for now. As documented:
For now, I will mark the issue as |
This looks like a regression to me, for many of the same reasons stated in the comments above. My primary reason for using ktlint is to format my code automatically so that it's readable. Of course consistency is important, but not at the expense of readability. It's also concerning that consistency is decided in a vacuum. @paul-dingemans you mention that:
which sounds nice, but comes with the presupposition that constructors and functions are both cars. Considering that's a matter of opinion, this probably shouldn't have been decided on unilaterally (or if there was more involvement, then it should've been in the open). |
We are using [*.{kt,kts}]
ktlint_standard_annotation = disabled because this part of this rule is simply unusable for our codebase. |
We've gotten around this for now by using the
Not only are constructors and functions "different beasts", primary constructors in Kotlin are even further removed from normal functions for several reasons:
Any of these would be good reason for potentially different formatting rules to apply to a primary constructor than to a function. And, indeed, the fact that it has to be part of the class header necessitates it. |
In past I have tried to get feedback on proposal for changes via the issue tracker and via Slack. This resulted in almost no feedback at all. The feedback collected in this issue is valuable (that is the reason that the issue was not closed), but unfortunately also not representative to the entire user group. Users that disagree with decisions made are much more likely to reach out than people who agree. I don't see any problem in |
Please see https://pinterest.github.io/ktlint/0.50.0/faq/#can-a-new-toggle-be-added-to-optionally-enabledisable-format-code-in-a-particular-way. Also note that when your
Yes, I am aware of that. Code style
Please feel free to create a PR which provides this clarity.
Above are indeed differences between constructors and functions. But the root cause of this issue is the handling of annotations on the constructor and how to format those annotations. According to the Kotlin Coding conventions:
|
That's just the way it is. Most people don't think about the tool that's going to format their code, and just assume it will work.
True, and it's also an effective way of getting feedback, but it also alienates users and lowers trust in the tool.
Android Kotlin Style Guide Neither of those are/were built with one person having unilateral decision power. It's also not very visible how much time and methodology went into making the decisions that they did. There's no way one person could have the context or info needed to make a decision that would apply across the ecosystem. I think you'll end seeing a lot more issues like this one crop up as more and more people adopt this style. Of course it would be good to get community involvement in the decision making process, but it's not a topic that's ever going to get broad community involvement. It's a rock and a hard place situation.
💯 and personally I'm very thankful that you do this. I try to contribute by testing new versions and filing issues when I could, but most of the time I have to work on my projects and don't want to spend time thinking about the formatting tool.
Absolutely, but with great power comes great responsibility. |
Yes, this is the general rule in the Kotlin coding conventions, but it then proceeds to explain two explicit exceptions in that same section:
And:
And this is probably where you're going to bring up "consistency" and we'll be going in circles. But we can just as easily be consistent by enforcing that a single annotation without arguments always goes on the same line. These are called out in the coding conventions for a reason, so it should at least be an option (enforce separate line, enforce same line, or do not enforce either way). |
How can I use the original format? The new line break before/after annotation and intend make the codes look so ugly. |
Any lint tools instead? @meyertime |
@hantsy Use the |
Comment copied from #2115, since I had missed this thread: Giving my feedback on this topic, I find it quite strange that classes with annotated constructor causes the class body to have 2 indents. All of a sudden I find some classes with 2 tabs and others with 1 tab. Instead of all classes looking similar, it seems quite random and does not give a feel of coherent code formatting in the project. Usually one does not look at the class constructor. Additionally because of the 2 tabs indent some lines now reach the max length and are formatted completely different, even though it's the same code within the same amount of scope levels. END of original comment. I don't even care that much about how the constructor looks like. Speaking of consistency mentioned in the thread, some classes having 2 tabs and some classes having 1 tab is not consistent and decreases readability. Indentation levels are usually a hint of scope level which is broken here. |
Coming from an earlier ktlint version this also looks like a big regression for me, but not so much because I (subjectively) dislike the new style; rather because once the code base is automatically reformatted (and our devs use that feature a LOT), we basically break git blame history on the respective files, and that's simply a no-go. Certainly, no one expected that a 0.x version of a software is still 100% compatible with a 1.x version, but if that 0.x version was out for years and people relied on it formatting their code in one particular way now see that the same tool in a stable version reformats their code completely, then this is not acceptable. Sorry, I see your intent and all the work that went into this, but please, please make this configurable. Thanks! |
I do understand that this is unwanted. Did you notice that the default code style of Ktlint was changed with publication of |
I had weird results locally when changing the different |
I am not familiar with the Kotlinter integration with ktlint. But I am happy for you that it works. |
Even though it's my fault for not reading the changelog prior to upgrading I would like to flag the this was the crucial bit that I missed. My motivation for upgrading to 1.x was to include a fix for #2343 so I was surprised that so many new violations are being raised after upgrading. After reconfiguring back to using |
This Please fix it, there is no rationale. |
Yes! There should never be a linting rule that causes the closing curly brace for a class to be indented differently from the beginning of the class definition. It is confusing when you get to the end of a file and it ends still indented. It also just wastes line real estate when everything in the class needs an extra level of indentation. |
This issue is marked as open/parked. What is the intention? Is more feedback being sought in other channels? It seems there enough options to work around this rule, but I am concerned that is the default rule and agree with comments regarding reduced readability and unexpected indentation. Its particularly stark coming from other languages and linting systems. |
The issue is parked as I am (still) not convinced by the arguments in this thread. The thread is kept open so that it is more conventient for users to find the thread, and to add additional insights. I am not actively researching the topic on any (other) channel. We have to bear in mind that only a limited number of developers out of thousands of users have objected against this way of formatting. Changing the current format as requested in this thread will make other developers unhappy. I am only willing to do so, when I am convinced so that I can explain/defend a new policy. |
Problem might be that ktlint eventually more and more moves away from the original intent of being a "non-bikeshedding" linter / formatter. Having different "styles" available already worked against that original intent, then allowing all rules to be configured / activated / deactivated worked against this as well and maybe there isn't even such a thing as an "non-bikeshedding" formatter and everything is fine as is. Maybe the official "ktlint" style is also just one biased version of a style and if we all agree to disagree on things and have ways to make "our own style" derived from whatever default style we're coming from, everybody is happy? In the end it's still just code formatting we're all heavily argueing about :) |
I do agree with most of what you say. Having different code styles for Android and non-Android is caused by having different sets of guidlines. The I am as restrictive as I can with configuration options for rules. Ideally I would get rid of all configuration properties, as well as the possibility to disable rules. But probably I would need to start an entire new project for that because the ktlint community will never be able to agree on one standard if they cannot tweak it to personal preferences. My advise is always to pick one of the code styles and to accept the default settings. It is up to the ktlint users whether they adhere to that. |
The irony is that this "anti-bikeshedding" linter doesn't actually eliminate bikeshedding; instead, the maintainer ends up dealing with all the bikeshedding whenever If you really want to avoid bikeshedding, then embrace configuration. Then each user can have their way with it, and there won't be any bikeshedding. Everyone is happy. Most languages have a configurable linter; that is what is lacking for Kotlin and where Of course, I'm sure the bikeshedding referred to in I picked up The only reason we're still using I know this isn't a forum to discuss the overall direction of the project, but I believe the issue here is a symptom of a deeper problem with the way things have been handled over the last year or two. But as far as the actual topic of this issue, I no longer have a pony in this race, because we're not using Finally, at a minimum, we have only asked for the option to configure this rule; you could do that and leave the default the way it is without affecting anyone who would supposedly be upset about the change. Though it would be better to improve the default. Most users would be pleased to see the readability of their code improve, and any outliers could configure it back to the way it was. Or, it could be implemented not to reformat code that was previously formatted the ugly way, but at the same time allow code that is formatted the nicer way. The point is, there are plenty of options that would avoid supposedly upsetting users that have been silent on this issue so far, so I don't buy that excuse. This is really telling: No other open issue on -- edit by @paul-dingemans : replaced references to my company with my user handle -- edit by @meyertime : changed Paul's edits so that they didn't change the meaning of what I was saying |
I resent you editing my comment. Your edit changed the point I was making. Why do you want to hide from this thread the fact that you don't work for Pinterest? It's in your public GitHub profile that you work for xxxx. I did not reveal any non-public information. Edit by @paul-dingemans : removed company name as not being relevant |
The company I work for is not relevant. It is is not a secret that I do not work for Pinterest. People can look up my resume easily. Although Pinterest has fostered this project for a couple of years, only one of the maintainers was employed by Pinterest when I became a maintainer. I removed my company name as you insinuate that my employer is relevant to this thread. All decisions I have made were my personal decisions. Please refrain from further escalations. According to The contribution guidelines of this project this should be a safe place for all. I did not attack you personally. But you are making it personal to me if you continue like this. |
Ok, I will concede that the name of your specific employer is not relevant to this discussion. I will respect your wishes and refrain from mentioning it, and I do apologize for my initial reaction. However, the fact that you are not employed by Pinterest is relevant, because it gives us a potential recourse before resorting to forking: We can appeal to Pinterest to take action. Further, while it may not be a "secret" that you don't work for Pinterest, it's also not common knowledge. I had assumed that the sole maintainer and contributor of this Pinterest-owned project was a Pinterest employee until I found out otherwise, and I believe that would be a common assumption. Also, holding you accountable for how you have been maintaining
You seem to be equating contributing with maintaining. Those are different roles. Yes, you've certainly done a lot of contributing. No one is saying otherwise. But maintaining is about making sure contributions are moving the project in the right direction. Sure, I've seen plenty of "open source" projects that are really just one person (the sole maintainer and contributor) putting something they made out there. They have their own vision for the project and will do what they want regardless. That person has every right to do whatever they want, as it's their project. However, it won't be truly useful to a lot of people and gain a significant following unless it satisfies the needs of the community rather than the maintainer's personal whim. However, this is not one of those projects. There was already a significant user base when you took over maintaining it. It didn't get that way because of your effort. Yet, you act as if your work on the project entitles you to do as you please and we should somehow just appreciate it. A maintainer going rogue isn't doing the community any favors, no matter how much they're contributing.
That is not true. If you refuse to accept clear feedback from the community, what reason do we have to believe that you would accept a PR that implements that feedback? At this point, appealing to Pinterest, or starting a community-driven fork of |
This has clearly become a quite spirited discussion. I recently upgraded package versions at work, and I came across this issue, so here I am sharing my thoughts for the first time ever on a GitHub issue. I am a bit perplexed regarding the resistance to making this rule configurable. There are other configurable rules in Ktlint already. There is clearly a desire from folks to have this configurable. This issue has more engagement than any other open issue of Ktlint, and it isn't particularly close. The current behavior can be left as a default. How is this in any way a bad thing or regression? @paul-dingemans, you said the following in this thread:
Code formatting is subjective. Given the above quotes and the fact that code formatting is purely subjective, is it fair to say that Ktlint just your personal preference on Kotlin code formatting? Is that the intended spirit of Ktlint? If the answer is yes to both, that's fine. If not, then it seems like adding some level of configurability for particularly controversial changes like this one would be a good solution. In my opinion, this package probably shouldn't be based on the preferences of a single person. I understand that you have an aversion to configuration properties based on your quote
But you immediately acknowledge that this idea isn't tenable:
Again, given the amount of push back on this particular formatting, how is this any different from other configuration properties? If the basis of other configuration properties comes from a lack of agreement on one standard, how is this meaningfully different? This entire thread is evidence of a lack of agreement upon a standard. Edit by @npouliquen: formatting |
Ok, so things got a little off the rails up there. I also opened #2703 to discuss the direction of As far as this issue is concerned, I will work on making the annotation rule configurable. If it goes well, I will drive an effort to do the same with the other rules. That way, there will be less reason to argue about the default behavior, since users that don't agree can just change their configuration. |
Please add you generic thoughts about configurability in discussion #2711. This current issue is intended solely for the configuration of the annotation wrapping before an explicit constructor. |
I just bumped into this issue when adding
This comment is spot on. I think ktlint has been taking tabs lightly recently. We should not have rules that change the entire indentation of a class that are not strictly related to the class itself. An "annotation" rule should not have the power to control anything outside the thing it's annotating. |
Expected Behavior
Our Android project contains many class declarations that look like this:
In my opinion, this should pass linting using the
ktlint_official
style. The official Kotlin and Android style guides actually contain no examples of annotations on a constructor, so I don't think they even thought about it when writing the style guide. However, the Kotlin guide does say, "A single annotation without arguments may be placed on the same line as the corresponding declaration," and the Android guide says something similar. Therefore, the above example should pass.Observed Behavior
Starting with ktlint v0.49.0, the above code produces the following linting errors:
Further, an auto-correction of the above code looks like this:
This is really ugly and hard to read. Linting should make our code better, not worse. The issue that introduced this feature suggests the following in the case of a short parameter list:
This is even worse. The indentation obscures where the opening brace is.
Steps to Reproduce
Run ktlint v0.49.0 or newer against the above code example with the
ktlint_official
style.Your Environment
ktlint_official
org.jmailen.kotlinter
version3.15.0
The text was updated successfully, but these errors were encountered: