Repository: bombsimon/wsl
Pull request #54 (2019-10-26 17:30:49)

Support to force/forbid ending newlines in case block
Addresses #49.

This is (maybe) a WIP/POC. I spent a lot of time on this since I couldn't get the logic to be easy enough to make this obvious. And just when I thought that I had something going on I realised that I should probably split this up in two settings.

1. Force newlines at
2. Forbid newlines before

I'm not sure how to combine these but I'll use this PR to iterate proposals.

coveralls: ## Pull Request Test Coverage Report for [Build 130](https://coveralls.io/builds/26568498)

* **92** of **92**   **(100.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**1.3%**) to **88.978%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/26568498/badge)](https://coveralls.io/builds/26568498) |
| :-- | --: |
| Change from base [Build 128](https://coveralls.io/builds/26568423): |  1.3% |
| Covered Lines: | 1114 |
| Relevant Lines: | 1252 |

---
##### 💛  - [Coveralls](https://coveralls.io)

bombsimon: Ok so now I've written my thoughts in #49 and in the commit but might as well add a short description here as well.

* `wsl` should focus on when to **add** whitespace, not remove them
* There are reasons to always allow trailing comments in case blocks so now we do that
* It's reasonable to mix traling newlines and not in case blocks so now we allow that
* It's now supported to force trailing newlines if a case block exceeds a set limit

Resovles #49 

Patch:
diff --git a/README.md b/README.md
index 11ba079..774feed 100644
--- a/README.md
+++ b/README.md
@@ -96,6 +96,7 @@ feel free to raise an [issue](https://github.com/bombsimon/wsl/issues/new).
 * [Assignments should only be cuddled with other assignments](doc/rules.md#assignments-should-only-be-cuddled-with-other-assignments)
 * [Block should not end with a whitespace (or comment)](doc/rules.md#block-should-not-end-with-a-whitespace-or-comment)
 * [Block should not start with a whitespace](doc/rules.md#block-should-not-start-with-a-whitespace)
+* [Case block should end with newline at this size](doc/rules.md#case-block-should-end-with-newline-at-this-size)
 * [Branch statements should not be cuddled if block has more than two lines](doc/rules.md#branch-statements-should-not-be-cuddled-if-block-has-more-than-two-lines)
 * [Declarations should never be cuddled](doc/rules.md#declarations-should-never-be-cuddled)
 * [Defer statements should only be cuddled with expressions on same variable](doc/rules.md#defer-statements-should-only-be-cuddled-with-expressions-on-same-variable)
diff --git a/cmd/wsl/main.go b/cmd/wsl/main.go
index 1463704..bcf900a 100644
--- a/cmd/wsl/main.go
+++ b/cmd/wsl/main.go
@@ -33,9 +33,9 @@ func main() {
 	flag.BoolVar(&config.StrictAppend, "strict-append", true, "Strict rules for append")
 	flag.BoolVar(&config.AllowAssignAndCallCuddle, "allow-assign-and-call", true, "Allow assignments and calls to be cuddled (if using same variable/type)")
 	flag.BoolVar(&config.AllowMultiLineAssignCuddle, "allow-multi-line-assign", true, "Allow cuddling with multi line assignments")
-	flag.BoolVar(&config.AllowCaseTrailingWhitespace, "allow-case-trailing-whitespace", false, "Allow case statements to end with an empty line")
 	flag.BoolVar(&config.AllowCuddleDeclaration, "allow-cuddle-declarations", false, "Allow declarations to be cuddled")
 	flag.BoolVar(&config.AllowTrailingComment, "allow-trailing-comment", false, "Allow blocks to end with a comment")
+	flag.IntVar(&config.CaseForceTrailingWhitespaceLimit, "force-case-trailing-whitespace", 0, "Force newlines for case blocks > this number.")
 
 	flag.Parse()
 
diff --git a/doc/configuration.md b/doc/configuration.md
index 391e937..34e3f5b 100644
--- a/doc/configuration.md
+++ b/doc/configuration.md
@@ -100,6 +100,52 @@ assignmentTwo := "so I cannot be cuddled"
 assignmentThree := "this is fine"
 ```
 
+### [force-case-trailing-whitespace](rules.md#case-block-should-end-with-newline-at-this-size)
+
+Can be set to force trailing newlines at the end of case blocks to improve
+readability. If the number of lines (including comments) in a case block exceeds
+this number a linter error will be yielded if the case does not end with a
+newline.
+
+> Default value: 0 (never force)
+
+Supported when set to 0:
+
+```go
+switch n {
+case 1:
+    fmt.Println("newline")
+
+case 2:
+    fmt.Println("no newline")
+case 3:
+    fmt.Println("Many")
+    fmt.Println("lines")
+    fmt.Println("without")
+    fmt.Println("newlinew")
+case 4:
+    fmt.Println("done")
+}
+```
+
+Required when set to 2:
+
+```go
+switch n {
+case 1:
+    fmt.Println("must have")
+    fmt.Println("empty whitespace")
+
+case 2:
+    fmt.Println("might have empty whitespace")
+
+case 3:
+    fmt.Println("might skip empty whitespace")
+case 4:
+    fmt.Println("done"9)
+}
+```
+
 ### [allow-cuddle-declarations](rules.md#declarations-should-never-be-cuddled)
 
 Controls if you're allowed to cuddle multiple declarations. This is false by
diff --git a/doc/rules.md b/doc/rules.md
index c0897d6..497cbd6 100644
--- a/doc/rules.md
+++ b/doc/rules.md
@@ -114,8 +114,7 @@ fmt.Println("x")
 ### Block should not end with a whitespace (or comment)
 
 > Can be configured, see [configuration
-documentation](configuration.md#allow-case-trailing-whitespace) ([for
-comments](configuration.md#allow-trailing-comment))
+documentation](configuration.md#allow-trailing-comment)
 
 Having an empty trailing whitespace is unnecessary and makes the block
 definition looks never-ending long. You want to let reader know that the
@@ -144,6 +143,39 @@ func example(y int) string {
 
 ---
 
+### Case block should end with newline at this size
+
+> Can be configured, see [configuration
+documentation](configuration.md#force-case-trailing-whitespace)
+
+To improve readability WSL can force to add whitespaces based on a set limit.
+See link to configuration for options.
+
+With `force-case-trailing-whitespace` set to 1 this yields an error.
+
+```go
+switch n {
+case 1:
+    fmt.Println("one")
+case 2:
+    fmt.Println("two")
+}
+```
+
+#### Recommended amendment
+
+```go
+switch n {
+case 1:
+    fmt.Println("one")
+
+case 2:
+    fmt.Println("two")
+}
+```
+
+---
+
 ### Block should not start with a whitespace
 
 Having an empty leading whitespace is unnecessary and makes the block definition
@@ -872,17 +904,6 @@ func IsNotZero(i int) string {
 
 ---
 
-### Stmt type not implemented
-
-Congratulations! You had found an unforseenable future detection. This hit
-simply means the detection is not implemented.
-
-#### Recommended amendment
-
-Raise an [issue](https://github.com/bombsimon/wsl/issues/new).
-
----
-
 ### Switch statements should only be cuddled with variables switched
 
 `switch` statements with associated switching variable should not cuddle with
diff --git a/wsl.go b/wsl.go
index 8a1f45b..ea8d4eb 100644
--- a/wsl.go
+++ b/wsl.go
@@ -50,21 +50,9 @@ type Configuration struct {
 	//  }
 	AllowMultiLineAssignCuddle bool
 
-	// AllowCaseTrailingWhitespace will allow case blocks to end with a
-	// whitespace. Sometimes this might actually improve readability. This
-	// defaults to false but setting it to true will enable the following
-	// example:
-	//  switch {
-	//  case 1:
-	//      fmt.Println(1)
-	//
-	// case 2:
-	//     fmt.Println(2)
-	//
-	// case 3:
-	//     fmt:println(3)
-	// }
-	AllowCaseTrailingWhitespace bool
+	// If the number of lines in a case block is equal to or lager than this
+	// number, the case *must* end white a newline.
+	CaseForceTrailingWhitespaceLimit int
 
 	// AllowTrailingComment will allow blocks to end with comments.
 	AllowTrailingComment bool
@@ -96,13 +84,13 @@ type Configuration struct {
 // DefaultConfig returns default configuration
 func DefaultConfig() Configuration {
 	return Configuration{
-		StrictAppend:                true,
-		AllowAssignAndCallCuddle:    true,
-		AllowMultiLineAssignCuddle:  true,
-		AllowCaseTrailingWhitespace: false,
-		AllowTrailingComment:        false,
-		AllowCuddleWithCalls:        []string{"Lock", "RLock"},
-		AllowCuddleWithRHS:          []string{"Unlock", "RUnlock"},
+		StrictAppend:                     true,
+		AllowAssignAndCallCuddle:         true,
+		AllowMultiLineAssignCuddle:       true,
+		AllowTrailingComment:             false,
+		CaseForceTrailingWhitespaceLimit: 0,
+		AllowCuddleWithCalls:             []string{"Lock", "RLock"},
+		AllowCuddleWithRHS:               []string{"Unlock", "RUnlock"},
 	}
 }
 
@@ -324,11 +312,15 @@ func (p *Processor) parseBlockStatements(statements []ast.Stmt) {
 				continue
 			}
 
-			if !atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) {
-				if !atLeastOneInListsMatch(assignedOnLineAbove, assignedFirstInBlock) {
-					p.addError(t.Pos(), "if statements should only be cuddled with assignments used in the if statement itself")
-				}
+			if atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) {
+				continue
 			}
+
+			if atLeastOneInListsMatch(assignedOnLineAbove, assignedFirstInBlock) {
+				continue
+			}
+
+			p.addError(t.Pos(), "if statements should only be cuddled with assignments used in the if statement itself")
 		case *ast.ReturnStmt:
 			if isLastStatementInBlockOfOnlyTwoLines() {
 				continue
@@ -751,8 +743,6 @@ func (p *Processor) findBlockStmt(node ast.Node) []*ast.BlockStmt {
 		blocks = append(blocks, p.findBlockStmt(t.Call)...)
 	case *ast.GoStmt:
 		blocks = append(blocks, p.findBlockStmt(t.Call)...)
-	default:
-		// spew.Dump(t)
 	}
 
 	return blocks
@@ -875,7 +865,10 @@ func (p *Processor) findLeadingAndTrailingWhitespaces(ident *ast.Ident, stmt, ne
 				break
 			}
 
-			allowedLinesBeforeFirstStatement += len(commentGroup.List)
+			// Support both /* multiline */ and //single line comments
+			for _, c := range commentGroup.List {
+				allowedLinesBeforeFirstStatement += len(strings.Split(c.Text, "\n"))
+			}
 		}
 	}
 
@@ -886,62 +879,92 @@ func (p *Processor) findLeadingAndTrailingWhitespaces(ident *ast.Ident, stmt, ne
 		)
 	}
 
-	// If the blockEndLine is 0 we're a case clause. If we don't have any
-	// nextStatement the trailing whitespace will be handled when parsing the
-	// switch. If we do have a next statement we can see where it starts by
-	// getting it's colon position.
-	if blockEndLine == 0 {
-		if nextStatement == nil {
-			return
+	// If the blockEndLine is not 0 we're a regular block (not case).
+	if blockEndLine != 0 {
+		if p.config.AllowTrailingComment {
+			if lastComment, ok := commentMap[lastStatement]; ok {
+				var (
+					lastCommentGroup = lastComment[len(lastComment)-1]
+					lastCommentLine  = lastCommentGroup.List[len(lastCommentGroup.List)-1]
+					countNewlines    = 0
+				)
+
+				countNewlines += len(strings.Split(lastCommentLine.Text, "\n"))
+
+				// No newlines between trailing comments and end of block.
+				if p.nodeStart(lastCommentLine)+countNewlines != blockEndLine-1 {
+					return
+				}
+			}
 		}
 
-		// If we allow case to end white whitespace or comment just return.
-		// TODO: Includes a false-positive where a newline before/after a
-		// comment in the case is valid but that's OK for now.
-		if p.config.AllowCaseTrailingWhitespace || p.config.AllowTrailingComment {
-			return
+		if p.nodeEnd(lastStatement) != blockEndLine-1 && !isExampleFunc(ident) {
+			p.addError(blockEndPos, "block should not end with a whitespace (or comment)")
 		}
 
-		switch n := nextStatement.(type) {
-		case *ast.CaseClause:
-			blockEndPos = n.Case
-		case *ast.CommClause:
-			blockEndPos = n.Case
-		default:
-			// We're not at the end of the case?
-			return
-		}
+		return
+	}
 
-		blockEndLine = p.fileSet.Position(blockEndPos).Line
+	// If we don't have any nextStatement the trailing whitespace will be
+	// handled when parsing the switch. If we do have a next statement we can
+	// see where it starts by getting it's colon position. We set the end of the
+	// current case to the position of the next case.
+	switch n := nextStatement.(type) {
+	case *ast.CaseClause:
+		blockEndPos = n.Case
+	case *ast.CommClause:
+		blockEndPos = n.Case
+	default:
+		// No more cases
+		return
 	}
 
-	if p.config.AllowTrailingComment {
-		if lastComment, ok := commentMap[lastStatement]; ok {
-			var (
-				lastCommentGroup = lastComment[len(lastComment)-1]
-				lastCommentLine  = lastCommentGroup.List[len(lastCommentGroup.List)-1]
-				countNewlines    = 0
-			)
-
-			if strings.Contains(lastCommentLine.Text, "\n") {
-				for _, x := range lastCommentLine.Text {
-					if x == '\n' {
-						countNewlines++
-					}
+	blockEndLine = p.fileSet.Position(blockEndPos).Line - 1
+
+	var (
+		blockSize                = blockEndLine - blockStartLine
+		caseTrailingCommentLines int
+	)
+
+	// TODO: I don't know what comments are bound to in cases. For regular
+	// blocks the last comment is bound to the last statement but for cases
+	// they are bound to the case clause expression. This will however get us all
+	// comments and depending on the case expression this gets tricky.
+	//
+	// To handle this I get the comment map from the current statement (the case
+	// itself) and iterate through all groups and all comment within all groups.
+	// I then get the comments after the last statement but before the next case
+	// clause and just map each line of comment that way.
+	for _, commentGroups := range commentMap {
+		for _, commentGroup := range commentGroups {
+			for _, comment := range commentGroup.List {
+				commentLine := p.fileSet.Position(comment.Pos()).Line
+
+				// Ignore comments before the last statement.
+				if commentLine <= p.nodeStart(lastStatement) {
+					continue
+				}
+
+				// Ignore comments after the end of this case.
+				if commentLine > blockEndLine {
+					continue
 				}
-			}
 
-			if p.nodeStart(lastCommentLine)+countNewlines == blockEndLine-1 {
-				return
+				// This allows /* multiline */ comments with newlines as well
+				// as regular (//) ones
+				caseTrailingCommentLines += len(strings.Split(comment.Text, "\n"))
 			}
 		}
 	}
 
-	if p.nodeEnd(lastStatement) != blockEndLine-1 && !isExampleFunc(ident) {
-		p.addError(
-			blockEndPos,
-			"block should not end with a whitespace (or comment)",
-		)
+	hasTrailingWhitespace := p.nodeEnd(lastStatement)+caseTrailingCommentLines != blockEndLine
+
+	// If the force trailing limit is configured and we don't end with a newline.
+	if p.config.CaseForceTrailingWhitespaceLimit > 0 && !hasTrailingWhitespace {
+		// Check if the block size is too big to miss the newline.
+		if blockSize >= p.config.CaseForceTrailingWhitespaceLimit {
+			p.addError(lastStatement.Pos(), "case block should end with newline at this size")
+		}
 	}
 }
 


Test patch:
diff --git a/wsl_test.go b/wsl_test.go
index b92bf5a..e4c5bc8 100644
--- a/wsl_test.go
+++ b/wsl_test.go
@@ -157,7 +157,6 @@ func TestShouldRemoveEmptyLines(t *testing.T) {
 			expectedErrorStrings: []string{
 				"block should not end with a whitespace (or comment)",
 				"block should not start with a whitespace",
-				"block should not end with a whitespace",
 			},
 		},
 		{
@@ -1067,7 +1066,6 @@ func TestShouldAddEmptyLines(t *testing.T) {
 				"block should not end with a whitespace (or comment)",
 				"block should not start with a whitespace",
 				"block should not start with a whitespace",
-				"block should not end with a whitespace (or comment)",
 			},
 		},
 		{
@@ -1309,27 +1307,6 @@ func TestWithConfig(t *testing.T) {
 			},
 			expectedErrorStrings: []string{"append only allowed to cuddle with appended value"},
 		},
-		{
-			description: "allow case ending whitespace",
-			code: []byte(`package main
-
-			func main() {
-				switch {
-				case 1:
-					fmt.Println(1)
-
-				case 2:
-					fmt.Println(1)
-
-				case 3:
-					// Still not allowed to end this block with a whitespace.
-					fmt.Println(1)
-				}
-			}`),
-			customConfig: &Configuration{
-				AllowCaseTrailingWhitespace: true,
-			},
-		},
 		{
 			description: "allow cuddle var",
 			code: []byte(`package main
@@ -1406,6 +1383,103 @@ func TestWithConfig(t *testing.T) {
 				"block should not end with a whitespace (or comment)",
 			},
 		},
+		{
+			description: "case blocks can end with or without newlines and comments",
+			code: []byte(`package main
+
+			func main() {
+				switch "x" {
+				case "a correct": // Test
+					fmt.Println("1")
+					fmt.Println("1")
+				case "a correct":
+					fmt.Println("1")
+					fmt.Println("2")
+
+				case "b correct":
+					fmt.Println("1")
+				case "b correct":
+					fmt.Println("1")
+
+				case "b bad":
+					fmt.Println("1")
+					// I'm not allowed'
+				case "b bad":
+					fmt.Println("1")
+					// I'm not allowed'
+
+				case "no false positives for if":
+					fmt.Println("checking")
+
+					// Some comment
+					if 1 < 2 {
+						// Another comment
+						if 2 > 1 {
+							fmt.Println("really sure")
+						}
+					}
+				case "end": // This is a case
+					fmt.Println("4")
+				}
+			}`),
+			customConfig: &Configuration{
+				CaseForceTrailingWhitespaceLimit: 0,
+				AllowTrailingComment:             false,
+			},
+		},
+		{
+			description: "enforce newline at size 3",
+			code: []byte(`package main
+
+			func main() {
+				switch "x" {
+				case "a correct": // Test
+					fmt.Println("1")
+				case "a ok":
+					fmt.Println("1")
+
+				case "b correct":
+					fmt.Println("1")
+					fmt.Println("1")
+					fmt.Println("1")
+
+				case "b bad":
+					fmt.Println("1")
+					fmt.Println("1")
+					fmt.Println("1")
+				case "b correct":
+					fmt.Println("1")
+					fmt.Println("1")
+					fmt.Println("1")
+					// This is OK
+
+				case "b bad":
+					fmt.Println("1")
+					fmt.Println("1")
+					fmt.Println("1")
+					// This is not OK, no whitespace
+				case "b ok":
+					fmt.Println("1")
+					fmt.Println("1")
+					fmt.Println("1")
+					/*
+						This is not OK
+						Multiline but no whitespace
+					*/
+				case "end": // This is a case
+					fmt.Println("4")
+				}
+			}`),
+			customConfig: &Configuration{
+				CaseForceTrailingWhitespaceLimit: 3,
+				AllowTrailingComment:             true,
+			},
+			expectedErrorStrings: []string{
+				"case block should end with newline at this size",
+				"case block should end with newline at this size",
+				"case block should end with newline at this size",
+			},
+		},
 	}
 
 	for _, tc := range cases {