Feature. Ktlint rule wrapper #744

aktsay6 · 2021-02-03T10:47:43Z

Introduced DiktatRule to wrap c.p.k.c.Rule

This pull request closes #606

### What's done: * Wrapper made * Class reworking hsa begun

### What's done: * Refactored more classes * Added error message

codecov · 2021-02-03T10:56:59Z

Codecov Report

Merging #744 (240d4a7) into master (d65d447) will decrease coverage by 0.42%.
The diff coverage is 93.23%.

@@             Coverage Diff              @@
##             master     #744      +/-   ##
============================================
- Coverage     80.05%   79.63%   -0.43%     
- Complexity     1997     2003       +6     
============================================
  Files            95       96       +1     
  Lines          5119     5032      -87     
  Branches       1644     1604      -40     
============================================
- Hits           4098     4007      -91     
- Misses          242      246       +4     
  Partials        779      779

Flag	Coverage Δ	Complexity Δ
unittests	`79.63% <93.23%> (-0.43%)`	`0.00 <78.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
.../ruleset/rules/chapter6/classes/DataClassesRule.kt	`68.75% <50.00%> (-1.25%)`	`30.00 <2.00> (ø)`
...et/rules/chapter6/classes/SingleConstructorRule.kt	`71.91% <50.00%> (-0.62%)`	`26.00 <1.00> (ø)`
.../diktat/ruleset/rules/chapter5/LambdaLengthRule.kt	`80.00% <66.66%> (-1.82%)`	`8.00 <1.00> (ø)`
...set/rules/chapter6/ExtensionFunctionsInFileRule.kt	`81.81% <66.66%> (-1.52%)`	`9.00 <2.00> (ø)`
...kotlin/org/cqfn/diktat/ruleset/rules/DiktatRule.kt	`75.00% <75.00%> (ø)`	`6.00 <6.00> (?)`
...rules/chapter3/LongNumericalValuesSeparatedRule.kt	`89.39% <80.00%> (-0.32%)`	`24.00 <1.00> (ø)`
...g/cqfn/diktat/ruleset/rules/chapter1/FileNaming.kt	`85.00% <100.00%> (-1.37%)`	`9.00 <1.00> (ø)`
.../diktat/ruleset/rules/chapter1/IdentifierNaming.kt	`74.66% <100.00%> (+0.34%)`	`84.00 <1.00> (ø)`
...qfn/diktat/ruleset/rules/chapter1/PackageNaming.kt	`91.76% <100.00%> (ø)`	`35.00 <1.00> (ø)`
...at/ruleset/rules/chapter2/comments/CommentsRule.kt	`90.47% <100.00%> (-0.30%)`	`26.00 <1.00> (ø)`
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d65d447...240d4a7. Read the comment docs.

petertrr · 2021-02-03T10:53:42Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRule.kt

+import org.cqfn.diktat.ruleset.utils.log
+import org.jetbrains.kotlin.com.intellij.lang.ASTNode
+
+typealias DiktatConfigRule = org.cqfn.diktat.common.config.rules.Rule


Suggested change

typealias DiktatConfigRule = org.cqfn.diktat.common.config.rules.Rule

private typealias DiktatConfigRule = org.cqfn.diktat.common.config.rules.Rule

Is it used only in this file?

petertrr · 2021-02-03T10:54:10Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRule.kt

+        else {
+            try {
+                logic(node)
+            } catch (e: Exception) {


Suggested change

} catch (e: Exception) {

} catch (e: Throwable) {

### What's done: * Fixed bugs

petertrr

Looks OK to me, @akuleshov7 please take a look. I remember you had some considerations whether to terminate application on internal errors.

### What's done: * Fixed bugs

orchestr7 · 2021-02-03T13:21:20Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRule.kt

+            try {
+                logic(node)
+            } catch (internalError: Throwable) {
+                log.error("Internal error has occurred in $id. Please make an issue on this bug at https://github.com/cqfn/diKTat/.\n Error: ${internalError.message}")


yes, exactly what @petertrr has mentioned. On the internal error we should stop diktat (exit 1) and SUGGEST to disable a rule where an issue happened

orchestr7 · 2021-02-03T13:22:50Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRule.kt

+        } else {
+            try {
+                logic(node)
+            } catch (internalError: Throwable) {


damn, catching all exceptions? I do not know if it is a good idea, @petertrr think about this here

I mean, isn't this what we were trying to achieve? Otherwise these exceptions are caught by ktlint and they just log a warning as well.

I am saying that simply doing catch (Exception e) is a bad practice. May be there are different types of exceptions in ktlint to handle BEFORE we will catch all remaining exeptions

Everything that is thrown from rule.visit is catched in Ktlint.kt with catch (e: Exception). So their idea is that the visitor either handles all exceptions, or they are considered internal errors.

orchestr7 · 2021-02-03T13:23:33Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter1/IdentifierNaming.kt

@@ -80,7 +79,11 @@ import org.jetbrains.kotlin.psi.psiUtil.parents
 * // FixMe: because it fixes only declaration without the usages
 */
 @Suppress("ForbiddenComment", "MISSING_KDOC_CLASS_ELEMENTS")
-class IdentifierNaming(private val configRules: List<RulesConfig>) : Rule("identifier-naming") {
+class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule("identifier-naming", configRules,
+    listOf(BACKTICKS_PROHIBITED, VARIABLE_NAME_INCORRECT, VARIABLE_NAME_INCORRECT_FORMAT, CONSTANT_UPPERCASE,


will this pass diktat formatting rule?

possible bug in diktat?

orchestr7 · 2021-02-03T13:26:37Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRule.kt

+@Suppress("TooGenericExceptionCaught")
+abstract class DiktatRule(id: String,
+                          val configRules: List<RulesConfig>,
+                          val rules: List<DiktatConfigRule>) : Rule(id) {


please rename "rules" to "inspections". Who was writing a white paper? Me or you?

orchestr7 · 2021-02-03T13:31:55Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRule.kt

+    }
+
+    private fun areRulesEnabled(): Boolean =
+            rules.none { configRules.isRuleEnabled(it) }


Please correct me if I'm wrong: here you check that ALL rules are enabled. And if one of them is not enabled you simply do not run the whole rule (that can contain other inspections)?

Please add a test (sorry if I have missed it)

Added test and renamed a function

### What's done: * Fixed bugs

petertrr

lgtm

petertrr

lgtm

orchestr7

lgtm

### What's done: * Fixed bugs

aktsay6 added 2 commits February 2, 2021 19:16

feature/ktlint-rule-wrapper(#606)

bd6e597

### What's done: * Wrapper made * Class reworking hsa begun

* feature/ktlint-rule-wrapper(#606)

84cf9d4

### What's done: * Refactored more classes * Added error message

aktsay6 added the enhancement New feature or request label Feb 3, 2021

aktsay6 requested review from petertrr and orchestr7 February 3, 2021 10:47

petertrr requested changes Feb 3, 2021

View reviewed changes

* feature/ktlint-rule-wrapper(#606)

019ea91

### What's done: * Fixed bugs

petertrr reviewed Feb 3, 2021

View reviewed changes

aktsay6 force-pushed the feature/ktlint-rule-wrapper(#606) branch 2 times, most recently from 351c9b0 to 019ea91 Compare February 3, 2021 12:19

aktsay6 added 6 commits February 3, 2021 15:32

feature/ktlint-rule-wrapper(#606)

a5d178e

### What's done: * Fixed bugs

feature/ktlint-rule-wrapper(#606)

988ed05

### What's done: * Fixed bugs

feature/ktlint-rule-wrapper(#606)

6ab619b

### What's done: * Fixed bugs

feature/ktlint-rule-wrapper(#606)

d72624f

### What's done: * Fixed bugs

feature/ktlint-rule-wrapper(#606)

b79fd2e

### What's done: * Fixed bugs

feature/ktlint-rule-wrapper(#606)

59b993d

### What's done: * Fixed bugs

orchestr7 reviewed Feb 3, 2021

View reviewed changes

orchestr7 requested changes Feb 3, 2021

View reviewed changes

aktsay6 added 3 commits February 3, 2021 19:50

feature/ktlint-rule-wrapper(#606)

b847b98

### What's done: * Fixed bugs

feature/ktlint-rule-wrapper(#606)

004a54b

### What's done: * Fixed bugs

feature/ktlint-rule-wrapper(#606)

aeb3242

### What's done: * Fixed bugs

aktsay6 requested review from petertrr and orchestr7 February 4, 2021 08:00

petertrr reviewed Feb 4, 2021

View reviewed changes

petertrr approved these changes Feb 4, 2021

View reviewed changes

orchestr7 approved these changes Feb 4, 2021

View reviewed changes

aktsay6 and others added 2 commits February 4, 2021 11:49

Merge branch 'master' into feature/ktlint-rule-wrapper(#606)

66663ec

feature/ktlint-rule-wrapper(#606)

240d4a7

### What's done: * Fixed bugs

aktsay6 merged commit e51031c into master Feb 4, 2021

aktsay6 deleted the feature/ktlint-rule-wrapper(#606) branch February 4, 2021 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature. Ktlint rule wrapper #744

Feature. Ktlint rule wrapper #744

aktsay6 commented Feb 3, 2021

codecov bot commented Feb 3, 2021 •

edited

Loading

petertrr Feb 3, 2021

petertrr Feb 3, 2021

petertrr left a comment

orchestr7 Feb 3, 2021 •

edited

Loading

aktsay6 Feb 4, 2021

orchestr7 Feb 3, 2021

petertrr Feb 3, 2021

orchestr7 Feb 3, 2021

petertrr Feb 3, 2021

orchestr7 Feb 3, 2021

orchestr7 Feb 4, 2021

orchestr7 Feb 3, 2021

aktsay6 Feb 4, 2021

orchestr7 Feb 3, 2021 •

edited

Loading

orchestr7 Feb 3, 2021

aktsay6 Feb 4, 2021

petertrr left a comment

petertrr left a comment

orchestr7 left a comment

	typealias DiktatConfigRule = org.cqfn.diktat.common.config.rules.Rule
	private typealias DiktatConfigRule = org.cqfn.diktat.common.config.rules.Rule

Feature. Ktlint rule wrapper #744

Feature. Ktlint rule wrapper #744

Conversation

aktsay6 commented Feb 3, 2021

codecov bot commented Feb 3, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertrr left a comment

Choose a reason for hiding this comment

orchestr7 Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orchestr7 Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertrr left a comment

Choose a reason for hiding this comment

petertrr left a comment

Choose a reason for hiding this comment

orchestr7 left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 3, 2021 •

edited

Loading

orchestr7 Feb 3, 2021 •

edited

Loading

orchestr7 Feb 3, 2021 •

edited

Loading