feat: minor version strategy rules #132
Conversation
| minorVersionBumpKtlint(project(":ktlint-rules:minor-version-strategy")) | ||
| } | ||
|
|
||
| tasks.register<JavaExec>("minorVersionBumpScan") { |
There was a problem hiding this comment.
nit/naming: validateMinorVersionBump?
ktlint-rules/build.gradle.kts
Outdated
| } | ||
|
|
||
| tasks.test { | ||
| useJUnitPlatform() |
There was a problem hiding this comment.
You shouldn't need this useJUnitPlatform() call
| if (autoCorrect) { | ||
| node.treeParent.treeParent?.let { | ||
| it.treeParent.removeChild(it) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we should remove autocorrect from this rule, deleting APIs should be an intentional action, not something our linter does
There was a problem hiding this comment.
It won't run on every lint, only when someone executes the new Gradle task (fixMinorVersionBump). We should delete deprecated APIs when they're scheduled for removal, why not automate it?
There was a problem hiding this comment.
Deleting APIs is a sensitive operation and should not be automated
There was a problem hiding this comment.
Agreed, we'll likely have to make other manual changes after deleting a deprecated API such as replacing its usage in calling functions.
There was a problem hiding this comment.
naming: ApisScheduledForRemovalRule is awkward and long, how about DeprecatedApiRule?
| * Creates a ktlint rule that detects APIs annotated with @DeprecatedUntilVersion for the specified versions. | ||
| * If autocorrect is enabled, the API will be deleted. | ||
| */ | ||
| fun apisScheduledForRemovalRule(major: Int, minor: Int): Rule = |
There was a problem hiding this comment.
I don't see how this rule provider function gets connected to the Gradle task. Where do we pass the major / minor versions?
There was a problem hiding this comment.
It's not used in the Gradle task, it's for the downstream ktlint-rules:minor-version-strategy modules to create the same rule with different major/minor versions.
There was a problem hiding this comment.
Yeah I saw that after reviewing your aws-sdk-kotlin PR. I wish the rule could be registered here for a given Project
ktlint-rules/build.gradle.kts
Outdated
| main { | ||
| dependencies { | ||
| implementation(libs.ktlint.cli.ruleset.core) | ||
| api(libs.ktlint.cli.ruleset.core) |
There was a problem hiding this comment.
Was this never needed as implementation?
There was a problem hiding this comment.
It worked as an implementation before, but now it's an API so downstream consumers of this module don’t also need to depend on libs.ktlint.cli.ruleset.core.
| emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit, | ||
| ) { | ||
| if (node.elementType == ElementType.ANNOTATION_ENTRY) { | ||
| val sdkVersion = System.getProperty("sdkVersion").split(".") |
There was a problem hiding this comment.
Diving deeper, do we have to use a system property here? There's no way to inspect the gradle.properties and pull out the sdkVersion from there?
There was a problem hiding this comment.
This code worked:
val gradleProperties = Properties().apply {
load(File("gradle.properties").inputStream())
}
val sdkVersion = gradleProperties.getProperty("sdkVersion")
3 participants
Issue #, if available:
N/A
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.