ktlint icon indicating copy to clipboard operation
ktlint copied to clipboard

MaxLineLength Rule Not Working After API Upgrade

Open gchallen opened this issue 3 years ago • 1 comments

Expected Behavior

The MaxLineLength rule should work when used in the RuleProvider context required by the new API:

val ruleProviders = setOf(
  // other rules
  RuleProvider { MaxLineLengthRule() }
)
// ...
KtLint.lint(
  KtLint.ExperimentalParams(
    //
    ruleProviders = ruleProviders,
  )
)

Migrated from previously using version 0.46:

val ruleSet = RuleSet(
  // other rules
  MaxLineLengthRule()
)
KtLint.lint(
  KtLint.ExperimentalParams(
    //
    ruleSets = listOf(ruleSet),
  )
)

Observed Behavior

The MaxLineLengthRule does not flag overly-long lines.

Steps to Reproduce

See above.

Part of me suspects that this may be due to changes in how the .editorconfig values are loaded. We are passing an editorConfigPath argument to the KtLint.ExperimentalParams call which contains an override for the maximum line length. However, even after removing that parameter a line a length that should exceed the defaults is not flagged by ktlint.

All the other rules seem to be working fine, although our test suites aren't designed to test the rules themselves, only our integration, so they are far from exhaustive.

Your Environment

  • Currently on ktlint 0.46 but trying to prepare for the breaking changes in 0.48. Version being tested is 0.47.1.
  • API consumer

Thanks in advance for the help! ktlint is a great tool.

gchallen avatar Sep 22 '22 13:09 gchallen

Please provide code sample and exact way in which KtLint is called.

paul-dingemans avatar Sep 22 '22 15:09 paul-dingemans

Here's the full file: https://github.com/cs125-illinois/jeed/blob/ktlint_upgrade/core/src/main/kotlin/KtLint.kt. The minimal test suite is here: https://github.com/cs125-illinois/jeed/blob/ktlint_upgrade/core/src/test/kotlin/TestKtLint.kt, with the failing test being "it should check kotlin sources with too long lines". Based on our minimal test suite certain other rules (like indentation) do seem to be working, but as mentioned above, this is in no way an exhaustive test suite of ktlint rules, rather simply intended to ensure that our integration is set up properly.

As another note, while RuleSet is documented as marked for removal in 0.48, it's already not working in 0.47, with all rules failing pretty spectacularly due to the state preservation changes that are incoming.

gchallen avatar Sep 24 '22 16:09 gchallen

From https://github.com/cs125-illinois/jeed/blob/ktlint_upgrade/core/src/main/kotlin/KtLint.kt#L56 I do understand that you create a temporary .editorconfig file which is passed to KtLint on line https://github.com/cs125-illinois/jeed/blob/ktlint_upgrade/core/src/main/kotlin/KtLint.kt#L249.

As described in Ktlint (0.47.0) release notes:

Default/alternative .editorconfig Parameter "ExperimentalParams.editorConfigPath" is deprecated in favor of the new parameter "ExperimentalParams.editorConfigDefaults". When used in the old implementation this resulted in ignoring all ".editorconfig" files on the path to the file. The new implementation uses properties from the "editorConfigDefaults"parameter only when no ".editorconfig" files on the path to the file supplies this property for the filepath.

API consumers can easily create the EditConfigDefaults by calling "EditConfigDefaults.load(path)" or creating it programmatically.

I do understand that it is misleading that the deprecated parameter no longer works the same as before. Probably it would have been better if it was removed instead of deprecated.

You need to specify the default editor config properties via param editorConfigDefaults or editorConfigOverride.

I can not yet get your project to work with specifying the editorConfigDefaults param. I will check that at a later time.

Replacing line https://github.com/cs125-illinois/jeed/blob/ktlint_upgrade/core/src/main/kotlin/KtLint.kt#L249 with below, will make the test pass:

                    editorConfigOverride = EditorConfigOverride.from(
                        DefaultEditorConfigProperties.maxLineLengthProperty to "100"
                    )

paul-dingemans avatar Sep 24 '22 19:09 paul-dingemans

KtLint uses the ec4j (EditorConfig for Java) library to parse the editor config files. This library throws a null pointer exception whenever a default editor config (via ktlint's editorConfigDefault parameter) is specified and the resource path of the source file looks like "path:MainKt.kt". Good news is, that it succeeds when it is specified as "path:./MainKt.kt".

My first suggestion would be to omit the fileName parameter in the KtLint.lint(...) and KtLint.format(...) commands in case of snippets. Secondly, make sure you pass an (absolute/relative) path instead of a file name without path. Do not forget to change the fromKotlin function as well as it contains a filename without path.

paul-dingemans avatar Sep 25 '22 08:09 paul-dingemans

Thanks for the response! This makes more sense now.

Is there a way to read an .editorconfig file into a EditorConfigOverride object? The reason that we're trying to use the file in the first place is to interoperate with projects that use ktlint on the command line, and therefore store their defaults in the normal way using an .editorconfig on the path. Essentially we want to be able to edit files and run ktlint from the command line, and then later enforce the same style guidelines when we run ktlint on in-memory strings programmatically. (Hopefully that makes some amount of sense.)

gchallen avatar Sep 27 '22 13:09 gchallen

There is not way to read an .editorconfig file into EditorConfigOverride. But even if that would exists, it is probably not what you want. A property value specified in editorConfigOverride will always override that property even in case it is specified in an .editorconfig which is found in the project.

The editorConfigDefaults property seems more suitable for your use case. Properties in this object will only be used in case no suitable .editorconfig file is found for the file that you're scanning. This property can be loaded from a file and would look like following in your project:

                            editorConfigDefaults = EditorConfigDefaults.load(
                                Paths.get(editorConfigPath)
                            )

Note that you will still need the hack with the absolute/relative path of the source file.

paul-dingemans avatar Sep 27 '22 15:09 paul-dingemans

Closed, due to lack of follow-up.

paul-dingemans avatar Nov 06 '22 19:11 paul-dingemans