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

[#250 clone] Run ktlint in process isolation + allow overriding ktlint version #279

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

mateuszkwiecinski
Copy link
Contributor

@mateuszkwiecinski mateuszkwiecinski commented Sep 10, 2022

Clone of #250 with my suggested improvements/changes

  1. new configurations will be automatically configured, even for custom tasks
  2. It's possible to override ktlint version
  3. @hrach's custom rules were migrated to new ktlint 0.47.1 apis
  4. I slightly changed the way errors are reported back
  5. Plugin itself won't expose ktlint dependency anymore to its consumer (it's defined as compileOnly now)
  6. default runtime ktlint version is kept in sync with the one compilation uses
  7. [new] It doesn't only uses ktlint APIs, but also resolves rules from the ktlint version provided via plugin extension

Fixes #280
Fixes #34

Base automatically changed from bump_kotlinter to master September 10, 2022 22:37
@mateuszkwiecinski
Copy link
Contributor Author

Potential blocker: gradle/gradle#21964

@Karn
Copy link

Karn commented Sep 30, 2022

Hey @mateuszkwiecinski! Looking forward to this PR! I'm wondering if you have considered just manually loading the custom rules using something like:

fun ktlintRulesetsFromClasspath(classpath: ConfigurableFileCollection): List<RuleSetProviderV2> {
    // Load the files from the classpath into a new ClassLoader
    @Suppress("DEPRECATION")
    val fileUris = classpath.map { it.toURL() }.toTypedArray()
    val classLoader = URLClassLoader(fileUris, Thread.currentThread().contextClassLoader)
    return ServiceLoader.load(RuleSetProviderV2::class.java, classLoader).toList()
}

Where classpath here is the ktlintClasspath. That way we can bypass the processIsolation issues, at least temporarily.

@mateuszkwiecinski mateuszkwiecinski changed the base branch from master to mateuszkwiecinski/migrate_to_junit5 October 1, 2022 10:51
@mateuszkwiecinski
Copy link
Contributor Author

mateuszkwiecinski commented Oct 1, 2022

@Karn Thanks for sharing the idea💡
I haven't considered any workarounds yet, mainly because the process isolation works just fine. I did some manual testing, and it all worked like a charm. The blocker I stumbled upon turned out not being related to the process isolation, but rather the quirky behavior of Gradle TestKit testing framework 🤷
I disabled problematic tests on Windows, and the PR should soon reach reviewable state :)

Base automatically changed from mateuszkwiecinski/migrate_to_junit5 to master October 3, 2022 15:47
@mateuszkwiecinski mateuszkwiecinski changed the title [#250 clone] Run ktlint in process isolation [#250 clone] Run ktlint in process isolation + allow overriding ktlint version Oct 9, 2022
.github/workflows/default.yml Outdated Show resolved Hide resolved
@@ -157,6 +157,7 @@ kotlinter {
reporters = arrayOf("checkstyle", "plain")
experimentalRules = false
disabledRules = emptyArray()
ktlintVersion = "x.y.z"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure how to reflect the ktlint version can be overriden. the text block above mentions these are the defaults, and the default value is the current ktlint version (the one kotlinter is compiled with, to be more precise)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative, I can propose having some sort of hardcoded value that will be mapped into "our" ktlint version, i.e.

Suggested change
ktlintVersion = "x.y.z"
ktlintVersion = "default" // or `latest` or `current`? but that might be too confusing

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually show the default value if you don't configure anything.
Which I suppose also causes a little confusion because people explicitly configure it not realizing that's what they get by default.

README.md Show resolved Hide resolved
build.gradle.kts Show resolved Hide resolved
build.gradle.kts Show resolved Hide resolved
Comment on lines 202 to 252
build("lintKotlin", "--info").apply {
assertEquals(SUCCESS, task(":lintKotlin")?.outcome)
assertTrue(output.contains("Resolved 45 RuleSetProviders"))
}

build("formatKotlin", "--info").apply {
assertEquals(SUCCESS, task(":formatKotlin")?.outcome)
assertTrue(output.contains("Resolved 45 RuleSetProviders"))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not ideal tests, and will likely fail when upgrading ktlint version, but I was out of ideas and decided to defer writing better assertion to somewhen later 😬
What I want to test here is to somehow replace the old test relying on test classpath, with something that covers the dynamic loading part.

test-project-android/app/build.gradle.kts Outdated Show resolved Hide resolved
@hrach
Copy link

hrach commented Dec 5, 2022

Friendly reminder - it would be great to have the functionality for custom rules. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants