Bump ktlint to 0.48.1 #304

lwasyl · 2022-12-16T15:21:35Z

I'm not actually sure this works, I just wondered how complex the update would be

.editorconfig

lwasyl · 2022-12-16T15:23:04Z

src/main/kotlin/org/jmailen/gradle/kotlinter/support/EditorConfigUtils.kt

    } else {
-        EditorConfigOverride.from(DefaultEditorConfigProperties.ktlintDisabledRulesProperty to rules.joinToString(separator = ","))
+        rules


This got hairy, since the disabled rules now are represented as a separate editorconfig properties, I had to do the same here. I'm not familiar with ec4j API but before I changed this a test failed and now it didn't so it looks like it works

🔧
My only suggestion here would be to add additional set of functional tests to ensure most common scenarios
i.e. experimental rules can also be disabled. I don't think we have that covered anywhere 👀

Well, good catch: I added a test for disabling an experimental rule (which is not in the standard ruleset) and it passed, but at the same time I added a positive test case to check that the experimental rule is executed in the first place if it's not disabled — it's not. This seems to be an unintended behavior change in ktlint 0.48.0, I assume it's fixed in 0.48.1. For now the PR will fail tests, hopefully after updating ktlint dependency to 0.48.1 it will pass.

As for using experimental rules: I understand it's not ideal and the tests would have to change as experimental rules evolve, but I didn't see an easy way to use a test-only custom rule set in the functional tests. If you have a suggestion on how to do that, I can try

0.48.1 is out now, so we can bump this PR to that perhaps?

I did that, but seems like either some artifacts are still not available or the CI is flaky — 16/21 checks passed, and quick look at the logs shows stuff I'm not sure is related to this PR 🤔

The tests were still failing, so I reproed and reported here: pinterest/ktlint#1768

Okay so, I think I've fixed the extension and I kept old behavior (using EditorConfigOverride), but after discussion in pinterest/ktlint#1768 I'm somewhat convinced that Kotlinter should not offer API to enable/disable rules, experimental or otherwise. And if it's really desireable, it shouldn't use EditorConfigOverride for that but rather EditorConfigDefaults.

Long explanation is here: paul-dingemans/ktlint@cfecedc

My take is that Ktlint respects hierarchical .editorconfig: you can override rules for each directory, while Kotlinter offers just a single global setting. The only reasonable behavior for Kotlinter is to provide its settings as a default rather than override, otherwise it's disallowing per-directory settings. But there are arguments to be made for removing Kotlinter API for configuring Ktlint completely:

users should have .editorconfig file in their project root directory even if they don't have any settings there, because without root = true Ktlint will search for .editorconfig outside of the project root directory (last I heard)

once there exists an .editorconfig file, it's reasonable to have it as a central source of configuration. Having global defaults/overrides in Kotlinter seems like a small benefit. Perhaps the API was added when it was more difficult (or impossible) to use .editorconfig, but clearly Ktlint considers it a sole source of configuration now and I think it's best not to fight it

I fully agree 👍 I'd strongly suggest moving away from EditorConfigOverride in favour of EditorConfigDefaults, as Paul suggested - it should be less confusing for the user.

Actually, I'd even consider taking a step further, and deprecate both disabled_rules and experimentalRules within the plugin, since it probably should be preferred to pass them via .editorconfig properties (wouldn't it?). My main motivation here is to get rid of some of the complexity we're adding, just to preserve backwards compatibility. Moreover, AFAICS configuring rules via .editorconfig requires that same amount of effort as via Plugin's extension, so the extension doesn't provide that much of a value anymore.
This might require a separate discussion though 👀

Yes, I agree. We should stop configuring rules altogether in our gradle config block. The config block remains but just for kotlinter specific gradle configurations such as ignore failures and reporters. A few consequences:

Our next kotlinter release will be 4.x compatibility breaking

We can include the other compatibility breaking changes in this release [#250 clone] Run ktlint in process isolation + allow overriding ktlint version #279 and Load Reporters dynamically, using provided ktlint version #294

Sounds like we need not wait for ktlint 0.48.2 since we don't need that editorconfig fix anymore?

Sounds like we need not wait for ktlint 0.48.2 since we don't need that editorconfig fix anymore?

Looks like it to me 🙂 Even with current implementation we don't need 0.48.2 I think, only 0.48.0 was breaking Kotlinter

@jeremymailen Please let me know how you'd like to proceed with this PR. Personally I'd prefer to merge it as is, as it's been open for a while now. Removing/deprecating rules configuration will probably involve writing some docs so I'd prefer to leave that to you (or at least a separate review)

lwasyl · 2022-12-16T15:24:29Z

src/main/kotlin/org/jmailen/gradle/kotlinter/support/EditorConfigUtils.kt

+    if (it.contains(':')) { // Rule from a custom source set
+        "ktlint_${it.replace(':', '_')}"
+    } else {
+        "ktlint_standard_$it"
+    }


Each disabled rule has to have ktlint_$ruleset prepended. From what I see, Kotlinter assumes standard default ruleset, I tried to keep that behavior

lwasyl · 2022-12-16T15:25:05Z

src/main/kotlin/org/jmailen/gradle/kotlinter/support/EditorConfigUtils.kt

+        "ktlint_standard_$it"
+    }
+
+internal fun KtLintRuleEngine.resetEditorconfigCacheIfNeeded(


KtLint is now deprecated in favor of KtLintRuleEngine instance, as far as I understand. I pulled it as a receiver where needed

src/main/kotlin/org/jmailen/gradle/kotlinter/tasks/LintTask.kt

lwasyl · 2022-12-16T15:26:46Z

src/main/kotlin/org/jmailen/gradle/kotlinter/tasks/format/FormatWorkerAction.kt

            changedEditorconfigFiles = parameters.changedEditorConfigFiles,
            logger = logger,
        )

        val fixes = mutableListOf<String>()
        try {
            files.forEach { file ->
-                val ruleSets = resolveRuleProviders(defaultRuleSetProviders, ktLintParams.experimentalRules)


I wasn't sure why rulesets were resolved for each file, was there a reason for that? Did I break something by moving rules resolution out of the loop?

I wasn't sure why rulesets were resolved for each file, was there a reason for that?

I'm not aware of any 🤷 I agree the change you proposed should work just fine 👀

You know I have a vague memory that they were stateful and if you didn't re-resolve them in the action you'd run into problems. Superstitiously I might recommend keeping the behavior. If you want to change it, make sure to run the PR version on a real project with a number of files and source sets.

Hah, based on the documentation it looks like you're right in that the rules might be stateful, but at the same time RuleSetProviderV2 solves statefulness issues (provided it's implemented as intended) and it's explicitly said that the engine won't reuse rules:

/** * The [Rule] contains the life cycle hooks which are needed for the KtLint rule engine. * * Implementation **doesn't** have to be thread-safe or stateless (provided that [RuleSetProviderV2] creates a new * instance of the [Rule] when [RuleSetProviderV2.getRuleProviders] has implemented its [RuleProvider] in such a way * that each call to [RuleProvider.createNewRuleInstance] indeed creates a new instance. The KtLint engine never * re-uses a [Rule] instance once is has been used for traversal of the AST of a file. */ public open class Rule(

It might be a good case to solve when adding tests involving custom rulesets though. I'll also check the current implementation on my $workProject 👍

Yeah, they may well have fixed this issue in later ktlint versions -- so long as it tests out ok, I'm good with a change here 👍

Actually, discovered you do in fact need to resolve rule providers for each file. If you don't, you get an error when KtLint tries to parse the .editorconfig on rule like

ktlint_standard_no-wildcard-imports = disabled

It will complain that ruleSetExecution is null in the VisitorProvider.

I'm putting together a 3.14.0 release and I'll fix there.

lwasyl · 2022-12-16T15:27:11Z

src/main/kotlin/org/jmailen/gradle/kotlinter/tasks/format/FormatWorkerAction.kt

+}

-    private fun formatKt(file: File, ruleSets: Set<RuleProvider>, onError: ErrorHandler) =
-        format(file, ruleSets, onError, false)
+private fun KtLintRuleEngine.formatKt(file: File, onError: ErrorHandler) =


Moved out of the file to keep function references working after I added a receiver

lwasyl · 2022-12-16T15:29:30Z

src/main/kotlin/org/jmailen/gradle/kotlinter/tasks/format/FormatWorkerAction.kt

-                fileName = file.path,
-                text = file.readText(),
-                ruleProviders = ruleProviders,
-                script = script,


Looks like ktlint now checks if the file is a script internally:

I missed that initially, looks like I can remove the kt/kts distinction from this file?

jeremymailen · 2022-12-17T22:27:50Z

Very much appreciate you working up the changes for the 0.48.0 update. I'll try to find some time over the next week or two to test and review. Thanks in advance for your patience -- some holiday travel coming up and work is extraordinarily hectic as well.

I've also noticed that ktlint intends to do a patch release soon to fix some regressions so we probably will want to jump to that version.

mateuszkwiecinski

Couple of minor comments, but otherwise LGTM 👍

.editorconfig

src/main/kotlin/org/jmailen/gradle/kotlinter/tasks/LintTask.kt

mateuszkwiecinski · 2022-12-21T10:05:11Z

src/main/kotlin/org/jmailen/gradle/kotlinter/tasks/format/FormatWorkerAction.kt

            changedEditorconfigFiles = parameters.changedEditorConfigFiles,
            logger = logger,
        )

        val fixes = mutableListOf<String>()
        try {
            files.forEach { file ->
-                val ruleSets = resolveRuleProviders(defaultRuleSetProviders, ktLintParams.experimentalRules)


I wasn't sure why rulesets were resolved for each file, was there a reason for that?

I'm not aware of any 🤷 I agree the change you proposed should work just fine 👀

src/main/kotlin/org/jmailen/gradle/kotlinter/support/EditorConfigUtils.kt

mateuszkwiecinski · 2022-12-21T10:24:10Z

src/main/kotlin/org/jmailen/gradle/kotlinter/support/EditorConfigUtils.kt

    } else {
-        EditorConfigOverride.from(DefaultEditorConfigProperties.ktlintDisabledRulesProperty to rules.joinToString(separator = ","))
+        rules


🔧
My only suggestion here would be to add additional set of functional tests to ensure most common scenarios
i.e. experimental rules can also be disabled. I don't think we have that covered anywhere 👀

jeremymailen · 2023-01-04T06:06:38Z

I'm back from break and have some time to help get this one through. Thanks again!

src/main/kotlin/org/jmailen/gradle/kotlinter/tasks/format/FormatWorkerAction.kt

mateuszkwiecinski · 2023-01-19T22:02:58Z

src/main/kotlin/org/jmailen/gradle/kotlinter/tasks/lint/LintWorkerAction.kt

-        resetEditorconfigCacheIfNeeded(
-            changedEditorconfigFiles = parameters.changedEditorconfigFiles,
+        val ktLintEngine = KtLintRuleEngine(
+            ruleProviders = resolveRuleProviders(defaultRuleSetProviders, ktLintParams.experimentalRules),


❓
Since experimental rules can be now also enabled via the editorconfig, shouldn't we always append them here?

Suggested change

ruleProviders = resolveRuleProviders(defaultRuleSetProviders, ktLintParams.experimentalRules),

ruleProviders = resolveRuleProviders(defaultRuleSetProviders, true),

I imagine it would be surprising to the users if they enabled experimental rules via .editorconfig, as stated in ktlint docs, and the rules wouldn't be used out-of-the-box.

So just to be clear, you're suggesting to change the plugin behavior for this version? I aimed at keeping compatibility and not introducing any new changes, but passing true here (in fact I can just remove this param whatsoever) will change the behavior 🤔 I thought it was possible to enable experimental rules already before, no?

I'd wait for @jeremymailen input here, I agree with you that we should load all rules, but at the same time I feel like doing that without moving to EditorConfigDefaults or deprecating Kotlinter rules configuration is a half-solution and might end up being more confusing. As a user, I'd prefer to have a single release which fully revamps the way I configure the rules

So just to be clear, you're suggesting to change the plugin behavior for this version?

Not really 🤔 I mean, the plugin is just a wrapper around ktlint. It avoids overriding default ktlint behavior. So if ktlint now allows enabling experimental rules via editorconfig, I don't see a reason why we would want to prevent that.

I thought it was possible to enable experimental rules already before, no?

As far as I understand, before 0.48.0, the only way to enable experimental rules was to pass additional RuleSet providers, which had to be done by the plugin. Now, given it's supported to enable rules via .editorconfig, if I added the ktlint_experimental=true, I'd expect the experimental rules to be enabled.
Screenshot from their documentation:

but passing true here will change the behavior

What behavior will change specifically? 🤔 Starting from 0.48.0 ktlint won't run experimental rules unless they are explicitly enabled, right?

Ah I thought it was possible to enable experimental rules via .editorconfig before as well. So yes, pulling all rules should not be harmful, but it will still be confusing if someone enables experimental rules in Kotlinter but wants to disable them only for some directory. We'd support some Ktlint configuration but not fully, it can get confusing

Done, all rules will now be passed to Ktlint

Yeah, as mentioned above, I support removing kotlinter's configuration of rules and delegating this entirely to editorconfig and ktlint's use of it. A couple assumptions I'm making

This harmonizes with people adding customer rule set to the build? I think so, just mentioning it

We properly set the gradle targets as outdated when various layers of editorconfig change -- I think we got this change last time around with the enhanced ktlint APIs if I remember right.

This harmonizes with people adding customer rule set to the build? I think so, just mentioning it

Afaiu yes, the rules will be added via Kotlinter mechanism and configured in .editorconfig. Though now that I think of it, I guess Ktlint might even want to load rules on the classpath itself

We properly set the gradle targets as outdated when various layers of editorconfig change

That I don't fully understand so can't answer

mateuszkwiecinski · 2023-01-19T22:11:41Z

src/main/kotlin/org/jmailen/gradle/kotlinter/support/EditorConfigUtils.kt

    } else {
-        EditorConfigOverride.from(DefaultEditorConfigProperties.ktlintDisabledRulesProperty to rules.joinToString(separator = ","))
+        rules


I fully agree 👍 I'd strongly suggest moving away from EditorConfigOverride in favour of EditorConfigDefaults, as Paul suggested - it should be less confusing for the user.

Actually, I'd even consider taking a step further, and deprecate both disabled_rules and experimentalRules within the plugin, since it probably should be preferred to pass them via .editorconfig properties (wouldn't it?). My main motivation here is to get rid of some of the complexity we're adding, just to preserve backwards compatibility. Moreover, AFAICS configuring rules via .editorconfig requires that same amount of effort as via Plugin's extension, so the extension doesn't provide that much of a value anymore.
This might require a separate discussion though 👀

mateuszkwiecinski · 2023-01-20T18:22:29Z

src/main/kotlin/org/jmailen/gradle/kotlinter/support/EditorConfigUtils.kt

+private fun getPropertiesForExperimentalRules(ktLintParams: KtLintParams) =
+    if (ktLintParams.experimentalRules) {
+        listOf(
+            EditorConfigProperty(
+                type = PropertyType("ktlint_experimental", "Experimental rules", IDENTITY_VALUE_PARSER),
+                defaultValue = "enabled",
+            ) to "enabled",
+        )
+    } else {
+        emptyList()
+    }


🔧 One last thing:
Since the goal of this PR is to support ktlint >=0.48, and they introduced a feature where you can hierarchically configure experimental rules via .editorconfig integration, I would strongly suggest to not block this behavior. With current state, if user enables experimental rules via plugin extension they won't be able to disable them via .editorconfig (i.e. for a specific directory). Having the "global" setting used as the default sounds much more reasonable to me.

Keeping scope of this PR small - I'd suggest keeping the editorConfigOverride for disabled rules (to preserve backwards compatibility), but start using editorConfigDefaults for experimental rules (since that's a new thing we're adding right now).

❔
And if the decision is to give plugin's extension priority, shouldn't we then force the disabled state as well? To serve a symmetrical behavior

Suggested change

private fun getPropertiesForExperimentalRules(ktLintParams: KtLintParams) =

if (ktLintParams.experimentalRules) {

listOf(

EditorConfigProperty(

type = PropertyType("ktlint_experimental", "Experimental rules", IDENTITY_VALUE_PARSER),

defaultValue = "enabled",

) to "enabled",

)

} else {

emptyList()

}

private fun getPropertiesForExperimentalRules(ktLintParams: KtLintParams) =

listOf(

EditorConfigProperty(

type = PropertyType("ktlint_experimental", "Experimental rules", IDENTITY_VALUE_PARSER),

defaultValue = "enabled",

) to if(ktLintParams.experimentalRules) "enabled" to "disabled",

)

mateuszkwiecinski

One last comment, the rest LGTM 🚀

jeremymailen

Yes, please proceed. We'll remove the rule configuration in a separate change.

lwasyl commented Dec 16, 2022

View reviewed changes

lwasyl force-pushed the update-ktlint-to-0.48.0 branch 3 times, most recently from 9964a57 to 8bb0bf7 Compare December 16, 2022 16:27

lwasyl marked this pull request as ready for review December 16, 2022 21:18

jeremymailen requested review from mateuszkwiecinski and jeremymailen December 17, 2022 22:25

mateuszkwiecinski linked an issue Dec 21, 2022 that may be closed by this pull request

KtLint 0.48.0 seems to have introduced a breaking change #305

Closed

mateuszkwiecinski reviewed Dec 21, 2022

View reviewed changes

lwasyl force-pushed the update-ktlint-to-0.48.0 branch 2 times, most recently from da97683 to c0a411f Compare January 2, 2023 08:44

lwasyl force-pushed the update-ktlint-to-0.48.0 branch from c0a411f to 2e3144e Compare January 4, 2023 16:47

lwasyl changed the title ~~Bump ktlint to 0.48.0~~ Bump ktlint to 0.48.1 Jan 4, 2023

lwasyl force-pushed the update-ktlint-to-0.48.0 branch from 2e3144e to 28c1404 Compare January 8, 2023 09:20

mateuszkwiecinski mentioned this pull request Jan 8, 2023

ktlint 0.48.1 doesn't use experimental rules if .editorconfig is empty pinterest/ktlint#1768

Closed

lwasyl force-pushed the update-ktlint-to-0.48.0 branch from 28c1404 to e020c7b Compare January 19, 2023 17:14

mateuszkwiecinski reviewed Jan 19, 2023

View reviewed changes

lwasyl added 2 commits January 20, 2023 11:24

Update KtLint to 0.48.1

bbf3e25

Always load experimental rules

bb1cc95

lwasyl force-pushed the update-ktlint-to-0.48.0 branch from e020c7b to bb1cc95 Compare January 20, 2023 10:32

mateuszkwiecinski reviewed Jan 20, 2023

View reviewed changes

Use EditorConfigDefaults to enable experimental rulesets

9f7fa87

mateuszkwiecinski approved these changes Jan 20, 2023

View reviewed changes

lwasyl force-pushed the update-ktlint-to-0.48.0 branch from d5eda7b to 9f7fa87 Compare January 20, 2023 21:05

jeremymailen requested review from jeremymailen and removed request for jeremymailen January 23, 2023 08:14

jeremymailen approved these changes Jan 23, 2023

View reviewed changes

mateuszkwiecinski merged commit 27ab24e into jeremymailen:master Jan 24, 2023

	ruleProviders = resolveRuleProviders(defaultRuleSetProviders, ktLintParams.experimentalRules),
	ruleProviders = resolveRuleProviders(defaultRuleSetProviders, true),

Bump ktlint to 0.48.1 #304

Bump ktlint to 0.48.1 #304

Conversation

lwasyl commented Dec 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwasyl Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymailen commented Dec 17, 2022

mateuszkwiecinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymailen commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszkwiecinski Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymailen Jan 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszkwiecinski left a comment

Choose a reason for hiding this comment

jeremymailen left a comment

Choose a reason for hiding this comment

lwasyl Jan 19, 2023 •

edited

Loading

mateuszkwiecinski Jan 20, 2023 •

edited

Loading

jeremymailen Jan 22, 2023 •

edited

Loading