-
Notifications
You must be signed in to change notification settings - Fork 2
fix: custom ktlint rules #130
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
Conversation
| attribute(Bundling.BUNDLING_ATTRIBUTE, objects.named(Bundling.SHADOWED)) | ||
| } | ||
| } | ||
| ktlint("aws.sdk.kotlin.gradle:ktlint-rules:$repoToolsVersion") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: "in case of custom ruleset"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule ID changes are to comply with Ktlint regex. e.g. see: https://github.com/pinterest/ktlint/blob/cd39c67816dee8c26b19a076eb064c97a248f670/ktlint-ruleset-template/src/main/kotlin/yourpkgname/NoVarRule.kt#L12
| import org.jetbrains.kotlin.com.intellij.lang.ASTNode | ||
|
|
||
| class MultilineIfElseBlockRule : Rule(RuleId("multiline-if-else-block"), About()) { | ||
| class MultilineIfElseBlockRule : Rule(RuleId("custom-ktlint-rules:multiline-if-else-block"), About()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/naming: aws-ktlint-rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about repo-tools?
The rules would be logged by ktlint like this:
Summary error count (descending) by rule:
repo-tools:copyright-header: 1
repo-tools:expression-body: 1
standard:indent: 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the full aws-kotlin-repo-tools-rules would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something with a -rules suffix for sure
| * @param lintPaths list of paths relative to the project root to lint (or not lint). | ||
| */ | ||
| fun Project.configureLinting(lintPaths: List<String>) { | ||
| fun Project.configureLinting(lintPaths: List<String>, repoToolsVersion: String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of having to keep track of aws-kotlin-repo-tools version in a second place. We have access to the Project, can't we dive in and find the aws-kotlin-repo-tools version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the caller side, if you can get the aws-kotlin-repo-tools version from libs.versions.toml and pass it to configureLinting, that would work too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of having to keep track of aws-kotlin-repo-tools version in a second place. We have access to the Project, can't we dive in and find the aws-kotlin-repo-tools version?
We don't keep track of the current version in repo tools like we do in the other repos so we can't do that unless we make bigger changes
From the caller side, if you can get the aws-kotlin-repo-tools version from libs.versions.toml and pass it to configureLinting, that would work too
That's what I'm planning to do, I made sure we can do so before I posted the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Project in this context is aws-sdk-kotlin, smithy-kotlin, aws-crt-kotlin which do declare a version of aws-kotlin-repo-tools in their libs.versions.toml file and should be accessible, but your other solution is fine too
That's what I'm planning to do
Ok 👍 , as long as we don't have to configure the aws-kotlin-repo-tools version in two places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get the version from inside configureLinting?
fun Project.configureLinting(lintPaths: List<String>) {
val repoToolsVersion = project
.configurations
.filter { it.isCanBeResolved }
.flatMap { it.resolvedConfiguration.resolvedArtifacts }
.map { it.moduleVersion.id }
.filter { it.group == "aws.sdk.kotlin.gradle" && it.name == "build-support" }
.map { it.version }
.distinct()
.single()
...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't as far as I know, this code returns an empty list. The list is already empty by the flatMap call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code worked:
val repoToolsVersion = extensions
.getByType<VersionCatalogsExtension>()
.named("libs")
.findVersion("aws-kotlin-repo-tools-version")
.get()
.requiredVersion
Issue #, if available:
N/A
Description of changes:
#42 broke our custom ktlint rules.
This PR fixes our custom ktlint rules
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.