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

Add rule to check that a single top level class name matches the file name #194

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package com.github.shyiko.ktlint.ruleset.standard

import com.github.shyiko.ktlint.core.KtLint
import com.github.shyiko.ktlint.core.Rule
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.psi.stubs.elements.KtStubElementTypes
import java.nio.file.Paths

/**
* If there is only one top level class in a given file, then its name should match the file's name
*/
class ClassNameMatchesFileNameRule : Rule("class-name-matches-file-name"), Rule.Modifier.RestrictToRoot {

override fun visit(
node: ASTNode,
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
) {
// Ignore all non ".kt" files (including ".kts")
if (node.getUserData(KtLint.FILE_PATH_USER_DATA_KEY)?.endsWith(".kt") != true) {
return
}

val topLevelClassNames = mutableListOf<String>()

node.visit {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's better to avoid whole AST traversal with something like

val topLevelClassNames = node.getChildren(null)
    .filter { it.elementType == KtStubElementTypes.CLASS }
    .mapNotNull { it.findChildByType(KtTokens.IDENTIFIER)?.text }

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 like this a lot more (removes the need to check for immediate parent and should also be more efficient :))

if (it.elementType == KtStubElementTypes.CLASS && it.treeParent.elementType == KtStubElementTypes.FILE) {
val classIdentifier = it.findChildByType(KtTokens.IDENTIFIER)
classIdentifier?.let {
val className = it.text
topLevelClassNames.add(className)
}
}
}

val name = Paths.get(node.getUserData(KtLint.FILE_PATH_USER_DATA_KEY)).fileName.toString().substringBefore(".")
Copy link
Collaborator

Choose a reason for hiding this comment

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

node.getUserData(KtLint.FILE_PATH_USER_DATA_KEY) should be extracted to a varible (referenced twice).

if (topLevelClassNames.size == 1 && name != topLevelClassNames.first()) {
emit(0,
"Single top level class name [${topLevelClassNames.first()}] does not match file name",
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about Class ${className} should be declared in a file named ${className}.kt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

false)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,4 @@ class NoMultipleSpacesRule : Rule("no-multi-spaces") {
}
}
}

private fun ASTNode.visit(cb: (node: ASTNode) -> Unit) {
cb(this)
this.getChildren(null).forEach { it.visit(cb) }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,4 @@ class NoUnusedImportsRule : Rule("no-unused-imports") {
}

private fun String.isComponentN() = componentNRegex.matches(this)

private fun ASTNode.visit(cb: (node: ASTNode) -> Unit) {
cb(this)
this.getChildren(null).forEach { it.visit(cb) }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,6 @@ class ParameterListWrappingRule : Rule("parameter-list-wrapping") {
return offsetToTheLeft + 1
}

private fun ASTNode.visit(cb: (node: ASTNode) -> Unit) {
cb(this)
this.getChildren(null).forEach { it.visit(cb) }
}

private fun errorMessage(node: ASTNode) =
when (node.elementType) {
KtStubElementTypes.VALUE_PARAMETER ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class StandardRuleSetProvider : RuleSetProvider {

override fun get(): RuleSet = RuleSet("standard",
ChainWrappingRule(),
ClassNameMatchesFileNameRule(),
FinalNewlineRule(),
// disabled until it's clear how to reconcile difference in Intellij & Android Studio import layout
// ImportOrderingRule(),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.github.shyiko.ktlint.ruleset.standard

import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafPsiElement
import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil
Expand All @@ -11,6 +12,10 @@ internal fun PsiElement.isPartOf(clazz: KClass<out PsiElement>) = getNonStrictPa
internal fun PsiElement.isPartOfString() = isPartOf(KtStringTemplateEntry::class)
internal fun PsiElement.prevLeaf(): LeafPsiElement? = PsiTreeUtil.prevLeaf(this) as LeafPsiElement?
internal fun PsiElement.nextLeaf(): LeafPsiElement? = PsiTreeUtil.nextLeaf(this) as LeafPsiElement?
internal fun ASTNode.visit(cb: (node: ASTNode) -> Unit) {
cb(this)
this.getChildren(null).forEach { it.visit(cb) }
}

internal fun <T> List<T>.head() = this.subList(0, this.size - 1)
internal fun <T> List<T>.tail() = this.subList(1, this.size)
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package com.github.shyiko.ktlint.ruleset.standard

import com.github.shyiko.ktlint.core.LintError
import com.github.shyiko.ktlint.test.lint
import org.assertj.core.api.Assertions.assertThat
import org.testng.annotations.Test

class ClassNameMatchesFileNameRuleTest {

@Test
fun testMatchingSingleClassName() {
assertThat(ClassNameMatchesFileNameRule().lint(
"""
class A
""".trimIndent(),
fileName("/some/path/A.kt")
)).isEmpty()
}

@Test
fun testNonMatchingSingleClassName() {
assertThat(ClassNameMatchesFileNameRule().lint(
"""
class B
""".trimIndent(),
fileName("A.kt")
)).isEqualTo(listOf(
LintError(1, 1, "class-name-matches-file-name", "Single top level class name [B] does not match file name")
))
}

@Test
fun testMultipleTopLevelClasses() {
assertThat(ClassNameMatchesFileNameRule().lint(
"""
class B
class C
""".trimIndent(),
fileName("A.kt")
)).isEmpty()
}

@Test
fun testMultipleNonTopLevelClasses() {
assertThat(ClassNameMatchesFileNameRule().lint(
"""
class B {
class C
class D
}
""".trimIndent(),
fileName("A.kt")
)).isEqualTo(listOf(
LintError(1, 1, "class-name-matches-file-name", "Single top level class name [B] does not match file name")
))
}

@Test
fun testCaseSensitiveMatching() {
assertThat(ClassNameMatchesFileNameRule().lint(
"""
interface Woohoo
""".trimIndent(),
fileName("woohoo.kt")
)).isEqualTo(listOf(
LintError(1, 1, "class-name-matches-file-name", "Single top level class name [Woohoo] does not match file name")
))
}

@Test
fun testIgnoreKotlinScriptFiles() {
assertThat(ClassNameMatchesFileNameRule().lint(
"""
class B
""".trimIndent(),
fileName("A.kts")
)).isEmpty()
}

private fun fileName(fileName: String) = mapOf("file_path" to fileName)
}