Add style rule for mandatory braces in for loop bodies #2658

daphil19 · 2020-05-07T17:15:09Z

This PR closes #2652

Extracted the description inside of 'MandatoryBracesIfStatements' to a constant to match the structure of 'MandatoryBracesForLoops'. Refactored the helper functions in 'MandatoryBracesIfStatements' to be extention functions, again similar to 'MandatoryBracesForLoops'. Refactored extension functions in both to explicitly use this to help make clear the properties of the receiver.

'MandatoryBracesIfStatements' and 'MandatoryBracesForloops' were not in alphabetical order.

schalkms

Thanks for tackling this and the nice 1st contribution!
The refactoring looks good.
The rule could finally be improved to also lint while loops and more tests.

schalkms · 2020-05-07T20:02:38Z

.../test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/MandatoryBracesForLoopsSpec.kt

+            }
+            """
+
+            assertThat(subject.lint(code)).isEmpty()


Please use the newer API. This allows to verify the correctness of given code snippets with the help of the Kotlin compiler.

Suggested change

assertThat(subject.lint(code)).isEmpty()

assertThat(subject.compileAndLint(code)).isEmpty()

schalkms · 2020-05-07T20:03:51Z

.../test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/MandatoryBracesForLoopsSpec.kt

+            assertThat(subject.lint(code)).isEmpty()
+        }
+
+        it("does report multi-line without braces") {


Suggested change

it("does report multi-line without braces") {

it("reports multi-line without braces") {

schalkms · 2020-05-07T20:07:16Z

.../src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/MandatoryBracesForLoops.kt

+ * for (i in 0..10) println(i)
+ * </compliant>
+ */
+class MandatoryBracesForLoops(config: Config = Config.empty) : Rule(config) {


It would make sense to also lint while loops. This adds further value to this rule.

Should that be a configuration item for this rule? Or should it be added as an additional rule?

There's no need for a configuration option here. Why would you want to have this rule enforced only for while but not for for loops? It makes very little sense.
Thus, the rule should flag both while and for loops.

schalkms · 2020-05-07T20:10:25Z

.../test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/MandatoryBracesForLoopsSpec.kt

+import org.spekframework.spek2.Spek
+import org.spekframework.spek2.style.specification.describe
+
+class MandatoryBracesForLoopsSpec : Spek({


The spec should contain more tests with edge cases.

Nested loops (with and without braces)

for with if-else (with and without braces)

BraisGabin · 2020-05-07T22:09:38Z

Can you add the test for for (a in b) println(a) this should not be flagged.

daphil19 · 2020-05-08T04:53:11Z

Can you add the test for for (a in b) println(a) this should not be flagged.

I think that the test case starting at line 25 in the Spek file covers this, right?

BraisGabin · 2020-05-08T06:57:17Z

Ups! I didn't see that one 👍🏻

This change addresses some of the commends in PR detekt#2658.

These tests were mainly edge cases that were requested in PR detekt#2658. While writing these tests, the actual MandatoryBracesForLoops rule was modified to add the apporpirate call to `super` in order to handle the nested loop case

The rule now checks for braces in both for and while loops. This commit also added unit tests for the while loop portion.

Because the rule now supports both for and while loops, the rule should be renamed from MandatoryBracesForLoops to MandatoryBracesLoops.

codecov · 2020-05-08T16:52:06Z

Codecov Report

Merging #2658 into master will decrease coverage by 0.02%.
The diff coverage is 69.56%.

@@             Coverage Diff              @@
##             master    #2658      +/-   ##
============================================
- Coverage     80.00%   79.97%   -0.03%     
- Complexity     2292     2303      +11     
============================================
  Files           379      380       +1     
  Lines          6761     6771      +10     
  Branches       1222     1225       +3     
============================================
+ Hits           5409     5415       +6     
  Misses          740      740              
- Partials        612      616       +4

Impacted Files	Coverage Δ	Complexity Δ
...ules/style/optional/MandatoryBracesIfStatements.kt	`64.28% <66.66%> (-7.94%)`	`14.00 <14.00> (ø)`
...etekt/rules/style/optional/MandatoryBracesLoops.kt	`69.23% <69.23%> (ø)`	`11.00 <11.00> (?)`
...bosch/detekt/rules/providers/StyleGuideProvider.kt	`100.00% <100.00%> (ø)`	`2.00 <0.00> (ø)`

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 57e745b...7bf1f36. Read the comment docs.

One of the while loop tests referenced a var that wasn't present.

schalkms · 2020-05-08T18:18:13Z

.../test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/MandatoryBracesForLoopsSpec.kt

+            fun test() {
+                // because if statements are expressions, this code properly prints "Odd" and "Even" 
+                for (i in 0..10) 
+                    if (i % 2 == 0) {
+                        println("Odd")
+                    } else {
+                        println("Even")
+                    }
+            }
+            """


What happens if you write code like this. Is the violation reported?

if (i % 2 == 0) for (i in 0..10) println(i) else { println("Even") }

schalkms

Looks good! I have another beneficial test case in my mind. After that I'll merge this with a smile on my face. 🙂

What happens if you write code like this. Is the violation reported?

if (i % 2 == 0)
  for (i in 0..10)
    println(i)
else {
  println("Even")
}

daphil19 added 5 commits May 7, 2020 12:22

Add style rule for mandatory braces in for loops

c24bbb4

Add unit tests to verify MandatoryBracesForLoops

f436ef9

Add daphil19 to contributors list in readme

88d9091

Reorder StyleGuideProvider list to be in alphabetical order

ea0319a

'MandatoryBracesIfStatements' and 'MandatoryBracesForloops' were not in alphabetical order.

schalkms reviewed May 7, 2020

View reviewed changes

daphil19 added 4 commits May 8, 2020 11:28

Refactor MandatoryBracesForLoops tests to use compileAndLint

761da24

This change addresses some of the commends in PR detekt#2658.

Add additonal test to MandatoryBracesForLoops

a7190b9

These tests were mainly edge cases that were requested in PR detekt#2658. While writing these tests, the actual MandatoryBracesForLoops rule was modified to add the apporpirate call to `super` in order to handle the nested loop case

Add while loop support for MandatoryBracesForLoops

0bf55be

The rule now checks for braces in both for and while loops. This commit also added unit tests for the while loop portion.

Rename MandatoryBracesLoops rule

fa83bb3

Because the rule now supports both for and while loops, the rule should be renamed from MandatoryBracesForLoops to MandatoryBracesLoops.

Fix reference error in test code for MandatoryBracesLoops

47650b9

One of the while loop tests referenced a var that wasn't present.

schalkms reviewed May 8, 2020

View reviewed changes

Add a test for loop in if statement for MandatoryBracesLoops

7bf1f36

schalkms approved these changes May 8, 2020

View reviewed changes

schalkms merged commit 1288153 into detekt:master May 8, 2020

arturbosch added this to the 1.8.1 milestone May 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add style rule for mandatory braces in for loop bodies #2658

Add style rule for mandatory braces in for loop bodies #2658

daphil19 commented May 7, 2020

schalkms left a comment •

edited

Loading

schalkms May 7, 2020

schalkms May 7, 2020

schalkms May 7, 2020 •

edited

Loading

daphil19 May 8, 2020

schalkms May 8, 2020

schalkms May 7, 2020

BraisGabin commented May 7, 2020

daphil19 commented May 8, 2020

BraisGabin commented May 8, 2020

codecov bot commented May 8, 2020 •

edited

Loading

schalkms May 8, 2020

schalkms left a comment

	assertThat(subject.lint(code)).isEmpty()
	assertThat(subject.compileAndLint(code)).isEmpty()

	it("does report multi-line without braces") {
	it("reports multi-line without braces") {

Add style rule for mandatory braces in for loop bodies #2658

Add style rule for mandatory braces in for loop bodies #2658

Conversation

daphil19 commented May 7, 2020

schalkms left a comment • edited Loading

Choose a reason for hiding this comment

schalkms May 7, 2020

Choose a reason for hiding this comment

schalkms May 7, 2020

Choose a reason for hiding this comment

schalkms May 7, 2020 • edited Loading

Choose a reason for hiding this comment

daphil19 May 8, 2020

Choose a reason for hiding this comment

schalkms May 8, 2020

Choose a reason for hiding this comment

schalkms May 7, 2020

Choose a reason for hiding this comment

BraisGabin commented May 7, 2020

daphil19 commented May 8, 2020

BraisGabin commented May 8, 2020

codecov bot commented May 8, 2020 • edited Loading

Codecov Report

schalkms May 8, 2020

Choose a reason for hiding this comment

schalkms left a comment

Choose a reason for hiding this comment

schalkms left a comment •

edited

Loading

schalkms May 7, 2020 •

edited

Loading

codecov bot commented May 8, 2020 •

edited

Loading