Add maxRuleVersion configuration for version-based rule filtering - #943
Add maxRuleVersion configuration for version-based rule filtering#943hugoncosta wants to merge 2 commits into
Conversation
|
For reference, the equivalent change in Ktlint has been merged - ktlint/ktlint#3101 |
| * @param maxVersion The maximum allowed version (from maxRuleVersion configuration) | ||
| * @return true if the rule version is compatible (should be included), false otherwise | ||
| */ | ||
| private fun isVersionCompatible(ruleVersion: String, maxVersion: String): Boolean { |
There was a problem hiding this comment.
we use net.swiftzer.semver.SemVer elsewhere in this project, so might as well use it here
|
Thanks for the contribution! |
|
Appreciate the feedback, I've gone ahead and implemented that, let me know your thoughts on it |
|
Looking good. one more question: how will this interact with versions of ktlint from before the annotation was retained at runtime? I think it should fail "open" in that case, and just include all rules. We support using ktlint versions back to 1.0.0, so this must be considered. |
|
That's a very good point - I was thinking more of in the future, if someone doesn't include it, but in practice, my change guarantees (with a reflection based test) that all rules will have the annotation, the issue is definitively the backwards compatibility, where some of them do not have it. I'll ensure that if it doesn't find it (the current scenario on 1.7.1 and below), it will accept all of them. I'll also make that clear in the javadoc of the new parameter. |
c6a9984 to
98d1e09
Compare
|
Ok actually the backwards compatibility was already baked in. I've updated the comments and added the @SInCE in the API itself, as the code was already merged and is currently planned as part of the 1.8.0 release |
JLLeitschuh
left a comment
JLLeitschuh
left a comment
There was a problem hiding this comment.
The tests are the big thing. They need to cover behavior, not just that the flags can/can't be set
| @DisplayName("Should allow setting maxRuleVersion") | ||
| @CommonTest | ||
| fun allowMaxRuleVersionConfiguration(gradleVersion: GradleVersion) { | ||
| project(gradleVersion) { | ||
| withCleanSources() | ||
| //language=Groovy | ||
| buildGradle.appendText( | ||
| """ | ||
|
|
||
| ktlint { | ||
| maxRuleVersion = "1.0.0" | ||
| } | ||
| """.trimIndent() | ||
| ) | ||
|
|
||
| build(CHECK_PARENT_TASK_NAME) { | ||
| assertThat(task(":$mainSourceSetCheckTaskName")?.outcome).isEqualTo(TaskOutcome.SUCCESS) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @DisplayName("Should work without maxRuleVersion set") | ||
| @CommonTest | ||
| fun workWithoutMaxRuleVersion(gradleVersion: GradleVersion) { | ||
| project(gradleVersion) { | ||
| withCleanSources() | ||
|
|
||
| build(CHECK_PARENT_TASK_NAME) { | ||
| assertThat(task(":$mainSourceSetCheckTaskName")?.outcome).isEqualTo(TaskOutcome.SUCCESS) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
So... These test that the maxRuleVersion can be set, but not that it behaves in a way that is expected. We should have a test for that.
There was a problem hiding this comment.
Totally agree, but for that, I think we'll need to park this PR until Ktlint 1.8.0 gets published, as the annotation isn't present at runtime (what we need) before. Or do you know of any other way to build against the unpublished Ktlint version?
| private fun isRuleCompatibleWithVersion(ruleProvider: RuleProvider, maxVersion: String): Boolean { | ||
| // Use reflection to check for @SinceKtlint annotation | ||
| val ruleClass = ruleProvider.createNewRuleInstance()::class.java | ||
| val sinceAnnotation = ruleClass.getAnnotation(SinceKtlint::class.java) | ||
|
|
||
| return if (sinceAnnotation != null) { | ||
| isVersionCompatible(sinceAnnotation.version, maxVersion) | ||
| } else { | ||
| // If no annotation, assume it's from an older ktlint version (before annotations were included | ||
| // at runtime) and include it for backward compatibility | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
This filtering isn't a feature built into ktlint itself as a library? I guess we are doing the rule loading manually, so I suppose this makes sense.
There was a problem hiding this comment.
Correct - per Paul's comment, it's the Ktlint Gradle plugin that provides the rules, so it's up to Ktlint Gradle to filter them out (if required). See ktlint/ktlint#3099 (comment)
|
Hello, happy new year, back to this as Paul has launched Ktlint 1.8.0 thus allowing me to finish the work here. Let me know your thoughts on the latest modifications |
| [versions] | ||
| kotlin = "1.5.31" | ||
| ktlint = "1.0.0" | ||
| ktlint = "1.8.0" |
There was a problem hiding this comment.
so the implementation of this would require us to compile against 1.8.0?
If so, we will need to wait until we are ready for a major version release to merge this feature.
If you would like this to be released sooner, I think isRuleCompatibleWithFilters would have to be reimplementated using reflection in order to be able to "work" (as a no-op) when older ktlint versions are used
There was a problem hiding this comment.
That's a good idea. I've gone ahead and implemented it using reflection, it's working as expected and doesn't force us to make a new major version :)
5826cdc to
b556b8b
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Hello, anything I can address in this to get it through? We have been using this internally in 1000s of packages successfully. |
3 participants
Address the feature request in #942
Summary
Adds
maxRuleVersionconfiguration to allow filtering rules based on their@SinceKtlintversion annotation.Changes
maxRuleVersionproperty toKtlintExtensionKtLintInvocation100based onmaxRuleVersionsettingMore context in the issue mentioned above.