Skip to content

Commit

Permalink
Added rule to detect commented code (#1)
Browse files Browse the repository at this point in the history
* Added rule to detect commented code

### What's done:
* Added CommentsRule with warning COMMENTED_OUT_CODE
* Added tests
* Fixed typo in all of custom rules
*Updated README.md
  • Loading branch information
petertrr committed Jul 3, 2020
1 parent a1d2e08 commit 0cdaf18
Show file tree
Hide file tree
Showing 13 changed files with 345 additions and 55 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,4 @@ To install git hooks using gradle run `gradle installGitHooks`.
|HEADER_MISSING_OR_WRONG_COPYRIGHT|Checks: copyright exists on top of file and is properly formatted (as a block comment). Fix: adds copyright if it is missing and required|
|HEADER_CONTAINS_DATE_OR_AUTHOR|Checks: header KDoc contains `@author` tag|
|HEADER_MISSING_IN_NON_SINGLE_CLASS_FILE|Check: warns if file with zero or >1 classes doesn't have header KDoc|
|COMMENTED_OUT_CODE|Check: warns if valid kotlin code is detected in commented blocks (both single-line and block comments)|
5 changes: 5 additions & 0 deletions diktat-rules/rules-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -173,5 +173,10 @@
"name": "HEADER_MISSING_IN_LONG_FILE",
"enabled": true,
"configuration": {}
},
{
"name": "COMMENTED_OUT_CODE",
"enabled": true,
"configuration": {}
}
]
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ enum class Warnings(private val id: Int, private val canBeAutoCorrected: Boolean
HEADER_MISSING_OR_WRONG_COPYRIGHT(37, true, "file header comment must include copyright information inside a block comment"),
HEADER_CONTAINS_DATE_OR_AUTHOR(38, false, "file header comment should not contain creation date and author name"),
HEADER_MISSING_IN_NON_SINGLE_CLASS_FILE(39, false, "files that contain multiple or no classes should contain description of what is inside of this file"),
COMMENTED_OUT_CODE(39, false, "you should not comment out code, use VCS to save it in history and delete this block"),
;

override fun ruleName(): String = this.name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import com.pinterest.ktlint.core.RuleSet
import com.pinterest.ktlint.core.RuleSetProvider
import org.cqfn.diktat.common.config.rules.RulesConfig
import org.cqfn.diktat.common.config.rules.RulesConfigReader
import org.cqfn.diktat.ruleset.rules.comments.CommentsRule

/**
* this constant will be used everywhere in the code to mark usage of Diktat ruleset
Expand All @@ -15,13 +16,14 @@ const val DIKTAT_RULE_SET_ID = "diktat-ruleset"
class RuleSetDiktat(val rulesConfig: List<RulesConfig>, vararg rules: Rule) : RuleSet(DIKTAT_RULE_SET_ID, *rules)

fun KtLint.Params.getDiktatConfigRules(): List<RulesConfig> {
return (this.ruleSets.find { it.id == DIKTAT_RULE_SET_ID } as RuleSetDiktat).rulesConfig?: listOf()
return (this.ruleSets.find { it.id == DIKTAT_RULE_SET_ID } as RuleSetDiktat).rulesConfig
}

class DiktatRuleSetProvider(private val jsonRulesConfig: String = "rules-config.json") : RuleSetProvider {
override fun get(): RuleSet {
return RuleSetDiktat(
RulesConfigReader().readResource(jsonRulesConfig)?: listOf(),
CommentsRule(),
KdocComments(),
KdocMethods(),
KdocFormatting(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class FileNaming : Rule("file-naming") {
val VALID_EXTENSIONS = listOf(".kt", ".kts")
}

private lateinit var confiRules: List<RulesConfig>
private lateinit var configRules: List<RulesConfig>
private lateinit var emitWarn: ((offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit)
private var fileName: String? = null
private var isFixMode: Boolean = false
Expand All @@ -37,7 +37,7 @@ class FileNaming : Rule("file-naming") {
autoCorrect: Boolean,
params: KtLint.Params,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit) {
confiRules = params.getDiktatConfigRules()
configRules = params.getDiktatConfigRules()
fileName = params.fileName
emitWarn = emit
isFixMode = autoCorrect
Expand All @@ -52,7 +52,7 @@ class FileNaming : Rule("file-naming") {
if (fileName != null) {
val (name, extension) = getFileParts()
if (!name.isPascalCase() || !VALID_EXTENSIONS.contains(extension)) {
FILE_NAME_INCORRECT.warnAndFix(confiRules, emitWarn, isFixMode, "$name$extension", 0) {
FILE_NAME_INCORRECT.warnAndFix(configRules, emitWarn, isFixMode, "$name$extension", 0) {
// FixMe: we can add an autocorrect here in future, but is there any purpose to change file or class name?
}
}
Expand All @@ -66,7 +66,7 @@ class FileNaming : Rule("file-naming") {
if (classes.size == 1) {
val className = classes[0].getFirstChildWithType(IDENTIFIER)!!.text
if (className != fileNameWithoutSuffix) {
FILE_NAME_MATCH_CLASS.warnAndFix(confiRules, emitWarn, isFixMode, "$fileNameWithoutSuffix$fileNameSuffix vs $className", 0) {
FILE_NAME_MATCH_CLASS.warnAndFix(configRules, emitWarn, isFixMode, "$fileNameWithoutSuffix$fileNameSuffix vs $className", 0) {
// FixMe: we can add an autocorrect here in future, but is there any purpose to change file name?
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class IdentifierNaming : Rule("identifier-naming") {
val BOOLEAN_METHOD_PREFIXES = setOf("has", "is")
}

private lateinit var confiRules: List<RulesConfig>
private lateinit var configRules: List<RulesConfig>
private lateinit var emitWarn: ((offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit)
private var isFixMode: Boolean = false

Expand All @@ -42,7 +42,7 @@ class IdentifierNaming : Rule("identifier-naming") {
params: KtLint.Params,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
) {
confiRules = params.getDiktatConfigRules()
configRules = params.getDiktatConfigRules()
isFixMode = autoCorrect
emitWarn = emit

Expand Down Expand Up @@ -77,7 +77,7 @@ class IdentifierNaming : Rule("identifier-naming") {
if (!ONE_CHAR_IDENTIFIERS.contains(variableName!!.text)) {
// generally variables with prefixes are not allowed (like mVariable)
if (variableName.text.hasPrefix()) {
VARIABLE_HAS_PREFIX.warnAndFix(confiRules, emitWarn, isFixMode, variableName.text, variableName.startOffset) {
VARIABLE_HAS_PREFIX.warnAndFix(configRules, emitWarn, isFixMode, variableName.text, variableName.startOffset) {
// FixMe: this correction should be done only after we checked variable case (below)
(variableName as LeafPsiElement).replaceWithText(variableName.text.removePrefix())
}
Expand All @@ -86,15 +86,15 @@ class IdentifierNaming : Rule("identifier-naming") {
// variable should not contain only one letter in it's name. This is a bad example: b512
// but no need to raise a warning here if length of a variable. In this case we will raise IDENTIFIER_LENGTH
if (variableName.text.containsOneLetterOrZero() && variableName.text.length > 1) {
VARIABLE_NAME_INCORRECT.warnAndFix(confiRules, emitWarn, isFixMode, variableName.text, variableName.startOffset) {
VARIABLE_NAME_INCORRECT.warnAndFix(configRules, emitWarn, isFixMode, variableName.text, variableName.startOffset) {
}
}

// check for constant variables - check for val from companion object or on global file level
// it should be in UPPER_CASE, no need to raise this warning if it is one-letter variable
if ((node.isNodeFromCompanionObject() || node.isNodeFromFileLevel()) && node.isValProperty() && node.isConst()) {
if (!variableName.text.isUpperSnakeCase() && variableName.text.length > 1) {
CONSTANT_UPPERCASE.warnAndFix(confiRules, emitWarn, isFixMode, variableName.text, variableName.startOffset) {
CONSTANT_UPPERCASE.warnAndFix(configRules, emitWarn, isFixMode, variableName.text, variableName.startOffset) {
(variableName as LeafPsiElement).replaceWithText(variableName.text.toUpperCase())
}
}
Expand All @@ -103,7 +103,7 @@ class IdentifierNaming : Rule("identifier-naming") {

// variable name should be in camel case. The only exception is a list of industry standard variables like i, j, k.
if (!variableName.text.isLowerCamelCase()) {
VARIABLE_NAME_INCORRECT_FORMAT.warnAndFix(confiRules, emitWarn, isFixMode, variableName.text, variableName.startOffset) {
VARIABLE_NAME_INCORRECT_FORMAT.warnAndFix(configRules, emitWarn, isFixMode, variableName.text, variableName.startOffset) {
// FixMe: cover fixes with tests
(variableName as LeafPsiElement).replaceWithText(variableName.text.toLowerCamelCase())
}
Expand All @@ -120,14 +120,14 @@ class IdentifierNaming : Rule("identifier-naming") {
private fun checkCLassNamings(node: ASTNode): List<ASTNode> {
val genericType: ASTNode? = node.getTypeParameterList()
if (genericType != null && !validGenericTypeName(genericType.text)) {
GENERIC_NAME.warnAndFix(confiRules, emitWarn, isFixMode, genericType.text, genericType.startOffset) {
GENERIC_NAME.warnAndFix(configRules, emitWarn, isFixMode, genericType.text, genericType.startOffset) {
// FixMe: should fix generic name here
}
}

val className: ASTNode? = node.getIdentifierName()
if (!(className!!.text.isPascalCase())) {
CLASS_NAME_INCORRECT.warnAndFix(confiRules, emitWarn, isFixMode, className.text, className.startOffset) {
CLASS_NAME_INCORRECT.warnAndFix(configRules, emitWarn, isFixMode, className.text, className.startOffset) {
(className as LeafPsiElement).replaceWithText(className.text.toPascalCase())
}
}
Expand All @@ -153,7 +153,7 @@ class IdentifierNaming : Rule("identifier-naming") {
?.text

if (superClassName != null && hasExceptionSuffix(superClassName) && !hasExceptionSuffix(classNameNode!!.text)) {
EXCEPTION_SUFFIX.warnAndFix(confiRules, emitWarn, isFixMode, classNameNode.text, classNameNode.startOffset) {
EXCEPTION_SUFFIX.warnAndFix(configRules, emitWarn, isFixMode, classNameNode.text, classNameNode.startOffset) {
// FixMe: need to add tests for this
(classNameNode as LeafPsiElement).replaceWithText(classNameNode.text + "Exception")
}
Expand All @@ -168,7 +168,7 @@ class IdentifierNaming : Rule("identifier-naming") {
val objectName: ASTNode? = node.getIdentifierName()
// checking object naming, the only extension is "companion" keyword
if (!(objectName!!.text.isPascalCase()) && objectName.text != "companion") {
OBJECT_NAME_INCORRECT.warnAndFix(confiRules, emitWarn, isFixMode, objectName.text, objectName.startOffset) {
OBJECT_NAME_INCORRECT.warnAndFix(configRules, emitWarn, isFixMode, objectName.text, objectName.startOffset) {
(objectName as LeafPsiElement).replaceWithText(objectName.text.toPascalCase())

}
Expand All @@ -185,7 +185,7 @@ class IdentifierNaming : Rule("identifier-naming") {
val enumValues: List<ASTNode> = node.getChildren(null).filter { it.elementType == ElementType.IDENTIFIER }
enumValues.forEach { value ->
if (!value.text.isUpperSnakeCase()) {
ENUM_VALUE.warnAndFix(confiRules, emitWarn, isFixMode, value.text, value.startOffset) {
ENUM_VALUE.warnAndFix(configRules, emitWarn, isFixMode, value.text, value.startOffset) {
// FixMe: add tests for this
(value as LeafPsiElement).replaceWithText(value.text.toUpperSnakeCase())
}
Expand All @@ -205,7 +205,7 @@ class IdentifierNaming : Rule("identifier-naming") {

// basic check for camel case
if (!functionName!!.text.isLowerCamelCase()) {
FUNCTION_NAME_INCORRECT_CASE.warnAndFix(confiRules, emitWarn, isFixMode, functionName.text, functionName.startOffset) {
FUNCTION_NAME_INCORRECT_CASE.warnAndFix(configRules, emitWarn, isFixMode, functionName.text, functionName.startOffset) {
// FixMe: add tests for this
(functionName as LeafPsiElement).replaceWithText(functionName.text.toLowerCamelCase())
}
Expand All @@ -217,7 +217,7 @@ class IdentifierNaming : Rule("identifier-naming") {
// if function has Boolean return type in 99% of cases it is much better to name it with isXXX or hasXXX prefix
if (functionReturnType != null && functionReturnType == PrimitiveType.BOOLEAN.typeName.asString()) {
if (!(BOOLEAN_METHOD_PREFIXES.any { functionReturnType.startsWith(it) })) {
FUNCTION_BOOLEAN_PREFIX.warnAndFix(confiRules, emitWarn, isFixMode, functionName.text, functionName.startOffset) {
FUNCTION_BOOLEAN_PREFIX.warnAndFix(configRules, emitWarn, isFixMode, functionName.text, functionName.startOffset) {
// FixMe: add agressive autofix for this
}
}
Expand Down Expand Up @@ -247,7 +247,7 @@ class IdentifierNaming : Rule("identifier-naming") {
isVariable: Boolean) {
nodes.forEach {
if (!(it.checkLength(2..64) || (ONE_CHAR_IDENTIFIERS.contains(it.text)) && isVariable)) {
IDENTIFIER_LENGTH.warn(confiRules, emitWarn, isFixMode, it.text, it.startOffset)
IDENTIFIER_LENGTH.warn(configRules, emitWarn, isFixMode, it.text, it.startOffset)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import org.jetbrains.kotlin.com.intellij.lang.ASTNode
*/
class KdocComments : Rule("kdoc-comments") {

private lateinit var confiRules: List<RulesConfig>
private lateinit var configRules: List<RulesConfig>
private lateinit var emitWarn: ((offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit)
private var isFixMode: Boolean = false

Expand All @@ -36,7 +36,7 @@ class KdocComments : Rule("kdoc-comments") {
params: KtLint.Params,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
) {
confiRules = params.getDiktatConfigRules()
configRules = params.getDiktatConfigRules()
emitWarn = emit
isFixMode = autoCorrect

Expand Down Expand Up @@ -72,7 +72,7 @@ class KdocComments : Rule("kdoc-comments") {
val name = node.getIdentifierName()

if (modifier.isAccessibleOutside() && kDoc == null) {
warning.warn(confiRules, emitWarn, isFixMode, name!!.text, node.startOffset)
warning.warn(configRules, emitWarn, isFixMode, name!!.text, node.startOffset)
}
}
}

0 comments on commit 0cdaf18

Please sign in to comment.