ktlint-gradle icon indicating copy to clipboard operation
ktlint-gradle copied to clipboard

Upgrade ktlint support

Open fthouraud opened this issue 2 years ago • 11 comments

Hi 👋

This MR is an attempt on upgrading support of Ktlint up to 0.46.1 (#589).

I started to work on this before this MR #593 arrived. So I opened a new one anyway at least to provide help.

I've split my work into steps in order to test each version from 0.42.1 to 0.46.1.

I also upgraded Gradle to 7.4.2 and Kotlin to 1.6.21.

fthouraud avatar Jul 02 '22 09:07 fthouraud

@JLLeitschuh how do you see the Ktlint support of this plugin? As is, it only supports the 0.46.1 due to the breaking changes.

If you still want to support the full range of versions from 0.34.0 then I think there should be a module for all versions prior to 0.46.1 and a second one for all versions from 0.46.1. The current plugin module could handle all common work which does not require references to a Ktlint that has been broken. This is a tremendous amount of work but doable I think.

IMHO, I think the support for the older versions could be dropped with a new major release of this plugin. Ktlint is a quality tool that should not harm any project by upgrading it. Moreover, it is configurable so that any new rule could be disabled.

What's your opinion on this?

fthouraud avatar Jul 15 '22 13:07 fthouraud

I guess one solution is to simply say that if someone requires backwards compatibility, they are welcome to contribute it.

Under that pretense, please be sure to clearly document that this will be a breaking change and move forward with the change 🙂

JLLeitschuh avatar Jul 17 '22 10:07 JLLeitschuh

I apologize for the delay, I got an increased amount of work lately so I'm lacking time on this.

I updated the PR with a new minimal supported version to 0.45.2. I've had to drop the 0.45.1 due to the lack of EditorConfigOverrides.

I also stepped back on the main property deprecation (JavaExecTask#main) as this would mean dropping the support for Gradle 6.0 but I think there's still time before the 8.0.

fthouraud avatar Jul 24 '22 16:07 fthouraud

I believe that this is now outdated by #597, correct?

JLLeitschuh avatar Aug 23 '22 15:08 JLLeitschuh

I believe that this is now outdated by #597, correct?

JLLeitschuh avatar Aug 23 '22 15:08 JLLeitschuh

@JLLeitschuh I don't get the point of the 11.0.0 release (which contains #597).:monocle_face: This only bumped Kotlin and Gradle. I thought the goal was to make ktlint-gradle ready for ktlint 0.46.+ (which this PR does)?

Maybe I am missing something?

G00fY2 avatar Aug 25 '22 07:08 G00fY2

I'll give this a look during the week-end. I should be able to rebase it.

That's progress anyway. Thanks.

fthouraud avatar Aug 25 '22 08:08 fthouraud

Maybe you can even check whether ktlint 0.47.0 works with your changes. :hearts:

G00fY2 avatar Aug 25 '22 09:08 G00fY2

I've managed to rebase this PR without trouble.

Regarding the upgrade to Ktlint 0.47.0, it will require more work than expected.

First, the RuleProvider interface is now deprecated and marked for deletion on 0.48.0. Its replacement, RuleProviderV2, is loadable using the same way (i.e. ServiceLoader). It's not very difficult to change that. I don't think it is reasonable to provide support for the old RuleProvider due to its imminent deletion.

The second change concerns the Baseline class which has been moved and refactored a bit. Nothing really difficult here.

Last, the generation of IDEA configuration is no longer provided by Ktlint. It is up to this plugin to also drop the support or to implement it directly.

I had some other failing tests I didn't look at closely but I don't think there are other significant issues.

I'll be glad to continue the upgrade process but I think this could be done in a separate MR due to the new changes it involves.

Please let me know what's your opinion on that.

fthouraud avatar Aug 28 '22 19:08 fthouraud

I've managed to rebase this PR without trouble.

Regarding the upgrade to Ktlint 0.47.0, it will require more work than expected.

First, the RuleProvider interface is now deprecated and marked for deletion on 0.48.0. Its replacement, RuleProviderV2, is loadable using the same way (i.e. ServiceLoader). It's not very difficult to change that. I don't think it is reasonable to provide support for the old RuleProvider due to its imminent deletion.

The second change concerns the Baseline class which has been moved and refactored a bit. Nothing really difficult here.

Last, the generation of IDEA configuration is no longer provided by Ktlint. It is up to this plugin to also drop the support or to implement it directly.

I had some other failing tests I didn't look at closely but I don't think there are other significant issues.

I'll be glad to continue the upgrade process but I think this could be done in a separate MR due to the new changes it involves.

Please let me know what's your opinion on that.

I'm certainly in favor of this. Thanks for your effort.

scottkennedy avatar Sep 02 '22 23:09 scottkennedy

It is up to this plugin to also drop the support or to implement it directly.

Dropping support in parity with the ktlint version seems reasonable. Especially if the major version was revised again and the minimum supported ktlint version was set to 0.47 (or perhaps 0.48 by the time this is merged). It would be a big, breaking change—but reducing the maintenance cost of this plugin seems valuable until it can get some new maintainers onboard.

There's also an argument to be made that IntelliJ should be responsible for its own configuration, so the kotlin plugin could provide an "official" and "android official" style preset, which would obviate the need for the ktlint/ktlint-gradle implementation.

Here's the relevant rationale upstream: pinterest/ktlint/issues/701

jzbrooks avatar Sep 07 '22 15:09 jzbrooks

Any update on this PR? Compose Rules requires ktlint versions over 0.46.0 🥲

dev-weiqi avatar Oct 12 '22 08:10 dev-weiqi

You might want to do something similar to https://github.com/diffplug/spotless/pull/1303 to ensure it's easy to maintain support for multiple versions of KtLint.

davidburstrom avatar Oct 14 '22 08:10 davidburstrom

I think we can close this PR, since I have added support for newer versions of ktlint in a way that is backward compatible via reflection

wakingrufus avatar Feb 02 '23 22:02 wakingrufus