Add support for ktlint to respect .editorconfig

[Voonder]

I found a bug with klint, especially the use of .editorconfig file.

When I use only klint the indend_size of the file is well use but not with spotless. Is there a configuration to put in spotless?

Below my config:

build.gradle

spotless {
	kotlin {
		ktlint('0.9.2')
	}
}

.editorconfig

root=true

[*.{kt,kts}]
indent_size=2

[nedtwigg]

Fixing this will require a change to KtLintStep and KotlinGradleExtension.

It will be necessary for the .editorconfig to be explicitly passed to spotless, e.g.

spotless {
    kotlin {
        ktlint().editorConfig('.editorconfig')
        ...

For an example of how to pass arguments like this in Spotless see ScalaExtension.ScalaFmtConfig.

PR's welcome!

[fvgh]

Just a quick remark: Like we discussed for #109, shouldn't the gradle interface look like

ktlint([version string]).configFile(<file location>)}

[nedtwigg]

Yep! That's why I linked to ScalaExtension.ScalaFmtConfig, because it is an example of that. I don't think it needs to necessarily be .configFile(), vs .editorConfig(). ktlint has multiple configuration options which we don't expose right now - .editorconfig isn't the only one. So it makes sense to name the arg after the specific configuration option rather than a generic configFile.

[runningcode]

I was trying to take a look at implementing this. I think the first step might be to get rid of the reflection in KtLintStep and move KtLintStep in to lib-extra and have it depend on ktlint directly. Does that make sense @nedtwigg ?

[runningcode]

Or, is it better to add the ktlint dependency as compileOnly?

[JLLeitschuh]

I wish it read the .editorconfig file by default TBH. But I understand from a major/minor version release semantic versioning standpoint that might not make sense.

[nedtwigg]

The reflection is ugly, but necessary, for reasons described in #187. It's fine to add it as a compile dep to work out what the code needs to be, but in the end it needs to be implemented in reflection. Make sure that the .editorconfig file is stored inside the FormatterStep's state, so that up-to-date checking works correctly.

[runningcode]

I was looking in to this a bit. It looks like ktlint only picks up the editorconfig file if it happens to be in the "ktlint working directory". In this case, this would be the spotlesscache? https://github.com/shyiko/ktlint/blob/db46520032207472cbdd90621c0f4f93ad5528d1/ktlint/src/main/kotlin/com/github/shyiko/ktlint/Main.kt#L260

[nedtwigg]

I think it would be the project directory, but I'm not positive. Regardless, Spotless would have to know about the .editorconfig file explicitly so that up-to-date checking can work. Perhaps KtLint would take a PR for an explicit .editorconfig?

[erikhofer]

Any progress on this?

[ZacSweers]

#469 will unlock this since KtLint 0.34 added a nicer API for passing this on to the formatting step

[ZacSweers]

I've been working on this, but think I've encountered a bug in KtLint - https://github.com/pinterest/ktlint/issues/605

If this behavior's intentional, Spotless would need to pass the file path and not just the contents to the format call. Note that this might be a good idea to support anyway, as I suspect file_name is something some custom rules could use

[ZacSweers]

Setting the file path could also make editorconfig support Just Work without needing that upstream fix, as it appears to (for better or worse) eagerly traverse the file system hierarchy up from the source file (if available) searching for an editorconfig file. In most projects, I suspect this would work and just leave a custom step configuration for projects that don't fit this pattern.

[nedtwigg]

Is the .editorConfig parsing hierarchical? Or is there only one .editorConfig at the project root, and that's it?

Spotless is fast because of up-to-date checking, but it's still pretty fast even if you disable that aspect (which each FormatterStep has the power to do if it chooses). If up-to-date checking is a little bit broken, it is going to consume hours of confused-human time, and it would be better to just disable the up-to-date checking. So we have to make sure to either not break up-to-date checking, or if we do break it, we need to make sure that KtLintStep properly disables up-to-date checking.

Normally we say that a step is String content -> String formattedContent, but it's actually String content, File path -> String formatterContent, but we usually try to hide the path.

https://github.com/diffplug/spotless/blob/c9dc6fc4311757a3466ef1b56d868a16b2996e1b/lib/src/main/java/com/diffplug/spotless/FormatterStep.java#L46

Here's an example of how to unhide the path

https://github.com/diffplug/spotless/blob/c9dc6fc4311757a3466ef1b56d868a16b2996e1b/lib-extra/src/main/java/com/diffplug/spotless/extra/wtp/EclipseWtpFormatterStep.java#L119-L141

The problem is, if you read the file's content from the path, rather than from the String content, then Spotless will break in surprising ways. So it's important to only use the File for its path (e.g. to parse package-info.java or module-info.java differently than a regular java file).

If the path is only being used to traverse up the filesystem and find .editorConfig files, that is okay, but it will break up-to-date checking when the .editorConfig file changes. You can either:

• disable up-to-date checking for ktlint tasks: https://github.com/diffplug/spotless/blob/c9dc6fc4311757a3466ef1b56d868a16b2996e1b/lib/src/main/java/com/diffplug/spotless/FormatterStep.java#L125

• or you can completely capture the state of the .editorconfig files inside KtLintStep.State.

My suggestion, but I don't know ktlint that well

If there's a way to preserve up-to-date checking, we should take that route! Even if .editorconfig has some hierarchical rules, I bet we can fake it with something like this pretty well:

spotless {
    kotlin {
        ktlint().editorConfig('.editorconfig')

I would store the binary or String content of the .editorConfig file inside KtLintStep.State, and I would make the path of .editorconfig a transient File. Then I would pass that .editorConfig path as the path to ktlint - if all ktlint is doing is searching directories, that ought to work. If you pass the real path, there's a risk that ktlint might (nor or in the future) read the content from there, and then Spotless will break.

If you really want to hierarchically find the .editorConfig automatically, that is possible, and we do it for .gitattributes, but it's very difficult.

[ZacSweers]

It is hierarchical, but will stop searching as soon as it reaches one that denotes itself as root = true. I think we may have to do it the hard way, as there doesn't appear to be an opt-out to prevent ktlint from performing this search regardless (if you pass in a file path). It's not toooooo hard to reproduce the search they use to find them. If anything it might be nice to ask them to make that public for build system tools like this. The more complicated step might be tying the up-to-date check per-dir since that's a theoretically possible level of granularity.

Or we take the easy way and just don't do up-to-date checks. We could make that an opt-in behavior for starters while iterating on re-enabling up-to-date checks too. Thoughts?

[nedtwigg]

One option is ktlint().editorConfig(), and .editorConfig() means "we search the project directory and all of its parents for .editorconfig (maybe also userhome? that seems very bad for CI)". It doesn't matter if we respect root = true, depending on more state than necessary is fine. When ktlint asks for a path, we always pass the project directory, guaranteeing that ktlint doesn't find any child .editorconfig, so that the up-to-date really does work.

You could extend this to ktlint().editorConfig('somedir1/', 'somedir2/') for the rare cases where people have per-directory formatting. But now, for each file you pass to ktlint, you'll have to find which "tip" in the state is closest to the file - much harder. Might as well do the full tree search at that point.

I think a good trade-off between implementation complexity and the universe of user needs:

• ktlint().editorConfigInThisOrParentDir() (easy to implement, works for most users, I would definitely merge this)
• ktlint().editorConfigAnywhereButDisablesUpToDateOptimization() (I would merge this iff somebody actually needs it. YAGNI.)

[nedtwigg]

Tracking issue for anybody who wants to remove reflection: https://github.com/diffplug/spotless/issues/524

[sdavids]

In the interim, please add a note to the https://github.com/diffplug/spotless/tree/master/plugin-gradle#applying-ktlint-to-kotlin-files section that .editorconfig will be ignored.

Also, continuation_indent_size is not supported by ktlint it is an IntelliJ-specific setting which has been renamed to ij_continuation_indent_size (https://blog.jetbrains.com/idea/2019/06/managing-code-style-on-a-directory-level-with-editorconfig/) -- you might want to remove it from the example.

[nedtwigg]

Drive-by documentation improvements are always welcome :)

[jcayzac]

I expected this not to depend at all on Spotless, and to simply work since ktlint has an --editorconfig option:

spotless {
  kotlin {
    target '**/*.kt'
    ktlint().userData(['editorconfig': "$rootDir/.editorconfig"])
  }
}

It doesn't work. Not sure why exactly…

[jcayzac]

It seems Spotless always sets the editorconfig param to null:

https://github.com/diffplug/spotless/blob/3a2620b17003dc56cc74ad1c2f3a6fcd3aea0af5/lib/src/main/java/com/diffplug/spotless/kotlin/KtLintStep.java#L171

Why is that?

[nedtwigg]

I expected this not to depend at all on Spotless, and to simply work

The reason Spotless is meddling is to support up-to-date and incremental formatting. In order to implement that, Spotless needs to capture the state of the files and the formatter setup.

It seems Spotless always sets the editorconfig param to null

Only because no one has done the work to implement it, it is not deliberately nerfed. My unconfirmed guess is that userData is meant to be a Map<String, String> of various config properties, but none of those properties are the location of the .editorConfig file, since there is an explicit parameter for it. The discussion above outlines a few different approaches which could add support for filling in the editorConfigPath, we'd be happy to merge any of them.

[cesards]

Any updates on this? I've tried different approaches and I can't figure out how to make Spotless use the .editorconfig .

[nedtwigg]

Now that #1012 has merged, it should be easier to build this if anybody wants to contribute a PR.

[ZacSweers]

fyi, this is is likely to get more interest since ktlint's latest release effectively breaks the current userData approach spotless relies on https://github.com/pinterest/ktlint/issues/1434

[nedtwigg]

Happy to take a PR for it :). I've switched to ktfmt and it's been great - stable APIs, less mucking with configuration.

[paul-dingemans]

From ktlint issue 605:

@ZacSweers How relevant is your question at this moment? I have been reading the spotless issue. If I understand correctly, the goal is that when Spotless invokes ktlint that .editorconfig is supported by ktlint.

As you remarked in comment, ktlint needs to know the path of the file so that it can check which .editorconfig files are located on the path to that file. The files are combined hierarchally where files deeper in the hierarchy are more important.

In case ExperimentalParams.editorConfigPath is specified, all '.editorconfig' files on the path to the file are being ignored. Only the setting from the ExperimentalParams.editorConfigPath are read and used.

I am open for suggestions to improve the API of ktlint but at this moment I can not figure out what Spotless's needs are given the current state of both Spotless and KtLint.

One possibily could be that the singular ExperimentalParams.editorConfigPath is changed to a list of paths to .editorConfig where files are ranked from most important to least important. This would allow spotless to collect and track the status of the files.

Another possibility is that an API is added which, given a directory path creates an instance of the EditorConfigOverride based on the '.editorConfig' files which then can be supplierd to calls of 'lint and format for all files in the directory.

@ZacSweers indicated in the issue that he no longer works with ktlint. As of that, I will close the ktlint issue. If any other participant in this thread is willing to pick this up, please let me know so that we can create a working solution.

[paul-dingemans]

In Ktlint 0.47 (to be released soon) the parameter 'alternativeEditorConfigis replaced bydefaultEditorConfig. The defaultEditorConfigjust holds editor config settings. A property value specified in those default will only be used in case that property is not found in any.editorconfig` on the path to the file. I believe this removes one of the biggest blockers of this issue. As API Consumer of KtLint, the defaultEditorConfig can be loaded once at consuming site (with a helper method provided by KtLint), and be passed as parameter to ktlint with each file being scanned.

[sudhirkhanger]

Is the gist of this discussion that the editorconfig rules are ignored by Spotless at the moment?

Secondly, if I do want to add editorconfig rules to Spotless then I will have to copy them as it is to the Gradle task.

indent_size = 2
ij_continuation_indent_size = 2
trim_trailing_whitespace = true
insert_final_newline = true
end_of_line = lf
max_line_length = 120

to

spotless {
  kotlin {
    target("**/*.kt")
    targetExclude("**/generated-sources/**")
    ktlint(libs.versions.ktlint.get())
      .editorConfigOverride([
        indent_size                : 2,
        ij_continuation_indent_size: 2,
        trim_trailing_whitespace   : true,
        insert_final_newline       : true,
        end_of_line                : "lf",
        max_line_length            : 120
      ])
  }
}

I can also assume the .editorConfigOverride honors most of the editorconfig configs.

[paul-dingemans]

I am not familiar with how ktlint is integrated in Spotless. Also it will depend on the specific version whether the .editorconfig is used or not.

Starting from version 0.47, KtLint has properties editorConfigDefaults and editorConfigOverride. A property defined in editorConfigDefaults will only be used in case the applicable .editorconfig for a a source file has not defined the property. When a property is defined in editorConfigOverride, the property value overrides the value found in the applicable .editorconfig.