Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Conversation

JelloRanger
Copy link
Contributor

This rule adheres to both Kotlin and Android Kotlin style guides:
https://kotlinlang.org/docs/reference/coding-conventions.html#source-file-names
https://android.github.io/kotlin-guides/style.html#naming

In order to make this change possible I had to plumb through the fileName into each Rule. This will also allow us to add additional style checks in the future (i.e. no underscores in file names, directory path matches package path, etc.)

Regarding autoFormat, I don't think there's a good solution here. If a violation occurs, then it's impossible for the tool to automatically determine whether to use the file name or the class name. The only alternative I can think of to this problem is to have Ktlint prompt the user for which name to use every time a violation occurs, which could be built after #79 is complete.

@JelloRanger JelloRanger changed the title Jelloranger/add single class matches filename rule Add rule to check that a single top level class name matches the file name Apr 22, 2018
shyiko added a commit that referenced this pull request Apr 22, 2018
@shyiko
Copy link
Collaborator

shyiko commented Apr 22, 2018

Hi Jacob (and thank you for the PR).

Everything inside ktlint-core/ is considered to be part of the public API and as such all changes must be backward compatible.

Starting from ktlint@0.22.0 file path is now accessible via
val path: String? = node.psi.containingFile.getUserData(KtLint.FILE_PATH_USER_DATA_KEY).

Could you please update the PR to use that instead?

Also,

  • ktlint-core/src/main/kotlin/com/github/shyiko/ktlint/core/ASTNodeExtensions.kt needs to be removed (core should stay minimal).
  • (ClassNameMatchesFileNameRule) emit(0, ...) instead of emit(topLevelClassNames.first().offset, ...) (this should allow to simplify rule to a few lines of code)
  • **/*.kts should not be checked.

@JelloRanger JelloRanger force-pushed the jelloranger/add-single-class-matches-filename-rule branch from 66d9966 to 1025de1 Compare April 30, 2018 00:08
@JelloRanger
Copy link
Contributor Author

Thanks for the feedback! (and adding the file path to userData :)) Updated the PR.


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 :))

}
}

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).

val name = Paths.get(node.getUserData(KtLint.FILE_PATH_USER_DATA_KEY)).fileName.toString().substringBefore(".")
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!

- Filter immediate children to get top level classes
- Changed error message
@shyiko
Copy link
Collaborator

shyiko commented May 1, 2018

Perfect. Thank you so much, Jacob!

@shyiko shyiko merged commit 79f5a79 into pinterest:master May 1, 2018
shyiko added a commit that referenced this pull request May 3, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants