-
Notifications
You must be signed in to change notification settings - Fork 503
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
Detect filename is not compliant with PascalCase convention #1117
Conversation
3308b6c
to
3f81397
Compare
hi @bedla thanks for the PR. Reading the Kotlin docs, my take is that filenames should always conform to Pascal case, regardless of the contents. Is that not also your understanding? |
92598a8
to
c343114
Compare
Yep, from my understanding PascalCase is only viable name of the file. I have rebased to current master. |
c343114
to
f2aef76
Compare
I have update PR to current master |
The code seems unnecessary complex to me. Basically, I would expect it do the following:
|
@bedla Do you plan to follow up on my remarks on this PR? |
Yes, I will. Sorry for being late, I had some rush in work. |
f2aef76
to
01f8f7c
Compare
Just pushed requested changes. I have rewritten original logic to make it more explicit from my point of view. Pls, take a look. Thx |
Please see commits for changes. The logic changes that I wanted was too difficult to describe in words, so I choose to implement it to check whether it worked. Code and tests did not comply with KtLints code formatting rules. So please ensure to also run the gradle "ktlint". Linting did pint out that also four KtLint files do not comply with the naming standard of this rule. I changed one of them, for which I had not objections to change it. But remaining three files, made me doubt about the rule (https://github.com/pinterest/ktlint/runs/5397719779?check_suite_focus=true):
https://kotlinlang.org/docs/coding-conventions.html#source-file-names is not very clear about files contain 1 toplevel function which is not a class. The current implementation enforces PascalName with the name of that toplevel function while the coding convention says that you should |
I see. Let me check it and add more tests. I will be at vacations next week, so result of my changes will be available after. |
Remove intermediary classes and functions which add complexity but do no assist in understanding the code more easily (although this is subjective of course).
Replace for-loops to "List.foreach { ... }"
…rsion" to "KtlintVersion.kt" with method "ktlintVersion" so that file adheres to PascalNaming convention in the modified FilenameRule.
25dd306
to
ffe22c6
Compare
Hmm, interesting "edge cases".
What do you thing? |
I think we should handle both cases similar. If a file does not contain any class and has filename ending with "Ext" or "Extension" and is following the PascalCase convention then there should be no violation about the file name. Would that be feasible? |
Ok, I will take a look at it. I have also created https://youtrack.jetbrains.com/issue/KT-51919 to know Kotlin creator's opinion on this edge case (I do not know if they will join discussion, but why not try :) ). |
Mr. Elizarov responded in Youtrack issue. I think that he is right. But it means that should remove checks around What do you think? |
It looks like that https://github.com/pinterest/ktlint/blob/0.45.2/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/FilenameRule.kt#L67 is just the example of a file in which extension functions have the same receiver. There can be a good reason to group such functions together in one file, but I agree that we should not force that the file is named after the receiver. It would be enough to enforce the upper PascalCase convention here. When applying above, a project can still use a filename like |
@bedla Do you have some time to finish up the issue? I expect a new release to be build in a couple of weeks. |
ahh, sorry I missed your previous comment. let me re-check it. |
I have removed that all-top-level receivers exception, but it still fails on:
So if I get it correctly, I should remove also this sub-rule for Extension functions & Extension property? What do you think? |
Yes they should be removed as well for two reasons:
|
Replace generic Util (e.g. Ext) name with a more descriptive name
There is one more file ("RuleExtension") which needs a better name. I will rename that file in the future as it is heavily refactored in another open PR. |
…/interface then the file should be named same as that class/interface. In all other cases a descriptive PascalCase name should be used. Modifications below were required to comply Ktlint code with change above: * Extract class KtlintCommandLine from file "Main.kt" to separate class * Rename files "main.kt" in Test resources to "Main.kt" * Rename file "BaselineSupport" to "CurrentBaseline" as the class CurrentBaseline is the dominant result of the file * Rename file "SuppressedRegionLocator" to "SuppressionLocatorBuilder". Functions had to be wrapped in an object as otherwise the file had to be named identical to the private data class SuppressionHint
# Conflicts: # ktlint/src/main/kotlin/com/pinterest/ktlint/Main.kt
Is it desired that with this change, the following file: package com.vanniktech.feature.boardmoney.data
import app.cash.sqldelight.adapter.primitive.IntColumnAdapter
import app.cash.sqldelight.db.SqlDriver
import com.vanniktech.boardmoney.QueryWrapper
internal fun createQueryWrapper(driver: SqlDriver) = QueryWrapper(
driver = driver,
boardMoneyPlayerAdapter = BoardMoneyPlayer.Adapter(
colorAdapter = IntColumnAdapter,
),
) gets linted with the following:
That does not seem right to me. |
It is debatable. According to the Kotlin styleguide a meaningfull name should be chosen. Depending on the context, |
I'd agree for a Class / Interface, but not for a given function (as in my case). |
Hi,
I found that FilenameRule does not check PascalCase convention mentioned in https://kotlinlang.org/docs/coding-conventions.html#source-file-names
What do you think?
Thx
Ivos