Skip to content
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

Double indentation of class body when superclass is wrapped #2423

Open
TWiStErRob opened this issue Dec 10, 2023 · 4 comments
Open

Double indentation of class body when superclass is wrapped #2423

TWiStErRob opened this issue Dec 10, 2023 · 4 comments
Labels
Parked Issue is not likely to be solved or has no clear resolution yet

Comments

@TWiStErRob
Copy link
Contributor

TWiStErRob commented Dec 10, 2023

Expected Behavior

Input is valid. Class body is correctly indented. Final brace is aligned with class opener.
Maybe the SuperClass(primaryParam) { line indented, but not the whole body. IntelliJ also just does this one line indent.

Observed Behavior

internal abstract class MyClass internal constructor(primaryParam: ResourceFolderRepository) :
    SuperClass(primaryParam) {
        private val privateProperty: File = primaryProperty.resourceDir

        fun doStuff() {
            val file = privateProperty.resolve("foo")
        }
    }

Steps to Reproduce

ktlint "MyClass.kt" --format

internal abstract class MyClass internal constructor(primaryParam: ResourceFolderRepository) :
SuperClass(primaryParam) {
    private val privateProperty: File = primaryProperty.resourceDir

    fun doStuff() {
        val file = privateProperty.resolve("foo")
    }
}

Note: when I unwrap the first line:

internal abstract class MyClass internal constructor(primaryParam: ResourceFolderRepository) : SuperClass(primaryParam) {

the indentation does not change at all, neither with:

internal abstract class MyClass internal constructor(
    primaryParam: ResourceFolderRepository
) : SuperClass(primaryParam) {

But this header also reproduces this issue:

internal abstract class MyClass internal constructor(primaryParam: ResourceFolderRepository)
: SuperClass(primaryParam) {

Your Environment

  • Version of ktlint used: 1.0.1
  • Relevant parts of the .editorconfig settings: none
  • Name and version (or code for custom task) of integration used (Gradle plugin, Maven plugin, command line, custom Gradle task): ktlint CLI
  • Version of Gradle used (if applicable): N/A
  • Operating System and version: Windows 10
@paul-dingemans
Copy link
Collaborator

@TWiStErRob
Copy link
Contributor Author

TWiStErRob commented Dec 10, 2023

What is the rationale behind re-indenting hundreds of lines based only on a very few lines of class header? I guess that part of the answer is that it matches some level of logical indentation, but which part? (I'm really curious about this!)


Here's a reason for not doing this:

When I'm looking at any random code at line 230, and see 2 levels of indentation it gives implicit context that the code is nested inside something. This rule takes away this crutch and requires anyone reading any code formatted with ktlint_official to actually have to look at the top level definition (which might be at the top of the file, or might be at line 145) to figure out if this is the case. This requires a lot of mental and physical (scrolling) gymnastics for something that's a given in 99.9% of auto-formatted code styles in a plethora of languages not just Kotlin.


Note: I might be actually fine with this, because I always advocate for the clearly structured:

class Name(
    param: Type, // any number of times
) : SuperClass(...) {

style which gives a clean and immediate overview without the need for brain-parsing the code on a single line into disparate pieces. And this style doesn't reproduce this issue. It just feels weird to reformat a whole file based on the placement of :

  • ) : Super -> "normal" indent
  • )\n: Super -> "double" indent
  • ) :\nSuper -> "double" indent

The class body logically belongs to the class, not the superclass constructor call, which means it should be always single-indented compared to the beginning of the class declaration. Having or not having a superclass or formatting thereof is irrelevant when it comes to the class body.

@paul-dingemans
Copy link
Collaborator

What is the rationale behind re-indenting hundreds of lines based only on a very few lines of class header? I guess that part of the answer is that it matches some level of logical indentation, but which part? (I'm really curious about this!)

The idea is to write all class headers in a consistent way. There are a few pain points when doing so:

  • annotations can be placed at many different elements in the class header. Not only on top of the class, but also on an explicit constructor, on type parameters, on class parameters, etc. From a consistency viewpoint, I want to handle annotions the same way, regardless of the construct on which they are used.
  • different indenting rules were applies on type parameters (with an indented >), versus class parameters (with de-indented ))

Is it worth to indent the entire class body compared to the size of the class header? That depends on your context and the size of your classes. For small classes it doesn't hurt at all. For big classes (code smell) it might actually hurt.

@diasDominik
Copy link

I also agree with @TWiStErRob the content belongs to the class and not the superclass so have the double indent makes it less clear were the code belongs to. Also I don't see a bigger class means that it is automatically a code smell(at which point is it actually a bigger class).

I'm all in for consistency but at the same time we should also not forget that the code should not get more complicated to read for humans.

Coming from python this indentiation would also be wrong so depends from where you come. But having it with a single indent is i believe in all languages the same

@paul-dingemans paul-dingemans pinned this issue Dec 19, 2023
@paul-dingemans paul-dingemans added the Parked Issue is not likely to be solved or has no clear resolution yet label Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Parked Issue is not likely to be solved or has no clear resolution yet
Projects
None yet
Development

No branches or pull requests

3 participants