Repository: bombsimon/wsl
Pull request #105 (2021-04-20 16:15:28)

#104 allow a blank line after each comment group at the start of a block
OK, figured it out. It's tricky, because the code allowed a space between groups, but we need to allow a space after each group. So the logic changes to take into account whether there's a break before the first group and to handle the case of just one group.

I've added a couple additional positive cases and a set of negative cases to show this works. Also added docs.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/38976383/badge)](https://coveralls.io/builds/38976383)

Coverage increased (+0.1%) to 91.972% when pulling **da626282ca2cd3c7d8d42dbdfd2b7641f3ea5eb3 on mholiday-nyt:104-comment-space** into **a6e08b33286b24ec81b47c17b60608e8b02a69f5 on bombsimon:master**.


Patch:
diff --git a/README.md b/README.md
index c5068a8..9812f94 100644
--- a/README.md
+++ b/README.md
@@ -121,5 +121,6 @@ feel free to raise an [issue](https://github.com/bombsimon/wsl/issues/new).
 * [Only one cuddle assignment allowed before type switch statement](doc/rules.md#only-one-cuddle-assignment-allowed-before-type-switch-statement)
 * [Ranges should only be cuddled with assignments used in the iteration](doc/rules.md#ranges-should-only-be-cuddled-with-assignments-used-in-the-iteration)
 * [Return statements should not be cuddled if block has more than two lines](doc/rules.md#return-statements-should-not-be-cuddled-if-block-has-more-than-two-lines)
+* [Short declarations should cuddle only with other short declarations](doc/rules.md#short-declaration-should-cuddle-only-with-other-short-declarations)
 * [Switch statements should only be cuddled with variables switched](doc/rules.md#switch-statements-should-only-be-cuddled-with-variables-switched)
 * [Type switch statements should only be cuddled with variables switched](doc/rules.md#type-switch-statements-should-only-be-cuddled-with-variables-switched)
diff --git a/cmd/wsl/main.go b/cmd/wsl/main.go
index 85dc070..414cb7c 100644
--- a/cmd/wsl/main.go
+++ b/cmd/wsl/main.go
@@ -38,6 +38,7 @@ func main() {
 	flag.BoolVar(&config.AllowTrailingComment, "allow-trailing-comment", false, "Allow blocks to end with a comment")
 	flag.BoolVar(&config.AllowSeparatedLeadingComment, "allow-separated-leading-comment", false, "Allow empty newlines in leading comments")
 	flag.BoolVar(&config.ForceCuddleErrCheckAndAssign, "force-err-cuddling", false, "Force cuddling of error checks with error var assignment")
+	flag.BoolVar(&config.ForceExclusiveShortDeclarations, "force-short-decl-cuddling", false, "Force short declarations to cuddle by themselves")
 	flag.IntVar(&config.ForceCaseTrailingWhitespaceLimit, "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 689524e..02d02a3 100644
--- a/doc/configuration.md
+++ b/doc/configuration.md
@@ -133,6 +133,36 @@ assignmentTwo := "so I cannot be cuddled"
 assignmentThree := "this is fine"
 ```
 
+### [allow-separated-leading-comment](rules.md#block-should-not-start-with-a-whitespace)
+
+This option allows whitespace after each comment group that begins a block.
+
+> Default value: false
+
+For example,
+
+```go
+func example() string {
+  // comment
+
+  return fmt.Sprintf("x")
+}
+```
+
+and
+
+```go
+func example() string {
+  // comment
+
+  // comment
+  return fmt.Sprintf("x")
+}
+```
+
+become legal, as the whitespace _after_ (or between) each comment block 
+doesn't count against whitespace before the first actual statement.
+
 ### [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
@@ -296,3 +326,41 @@ if err != nil {
     return err
 }
 ```
+
+### [force-short-decl-cuddling](rules.md#short-declaration-should-cuddle-only-with-other-short-declarations)
+
+Enforces that an assignment which is actually a short declaration (using `:=`)
+is only allowed to cuddle with other short declarations, and not plain
+assignments, blocks, etc. This rule helps make declarations stand out by
+themselves, much the same as grouping `var` statements.
+
+> Default value: false
+
+Supported when false:
+
+```go
+a := 1
+a = 2
+
+err := ErrorProducingFunc()
+if err != nil {
+    return err
+}
+```
+
+Required when true:
+
+```go
+a := 1
+
+a = 2
+
+err := ErrorProducingFunc()
+
+if err != nil {
+    return err
+}
+```
+
+**Note**: this means the option _overrides_ the 
+[enforce-err-cuddling](#enforce-err-cuddling) option above, among others.
diff --git a/doc/rules.md b/doc/rules.md
index 1c81d82..865bcf3 100644
--- a/doc/rules.md
+++ b/doc/rules.md
@@ -202,6 +202,31 @@ func example() string {
 }
 ```
 
+> However, this can be configured to allow white space after one
+> or more initial comment groups, see
+[configuration documentation](configuration.md#allow-separated-leading-comment)
+>
+> If that is done, then these examples are allowed:
+
+```go
+  func example() string {
+  // comment
+
+  return fmt.Sprintf("x")
+}
+```
+
+> and
+
+```go
+  func example() string {
+  // comment
+
+  // comment
+  return fmt.Sprintf("x")
+}
+```
+
 ---
 
 ### Branch statements should not be cuddled if block has more than two lines
@@ -1013,3 +1038,38 @@ if err != nil {
     return err
 }
 ```
+
+---
+
+### Short declaration should cuddle only with other short declarations
+
+> Can be configured, see [configuration
+documentation](configuration.md#force-short-decl-cuddling)
+
+A short declaration (an "assignment" using `:=`) must not be cuddled with anything other than another short declaration.
+
+```go
+a := 1
+err := ErrorProducingFunc()
+if err != nil {
+    return err
+}
+```
+
+#### Recommended amendment
+
+Add an empty line between the short declaration and the error check.
+
+```go
+a := 1
+err := ErrorProducingFunc()
+
+if err != nil {
+    return err
+}
+```
+
+**Note**: this is the opposite of the case above forcing an `err` variable
+to be assigned together with its check; it also overrides some other rules
+which about assignments by separating short declarations from "plain"
+assignment statements using `=`.
diff --git a/wsl.go b/wsl.go
index ae0dfb4..313b527 100644
--- a/wsl.go
+++ b/wsl.go
@@ -41,6 +41,7 @@ const (
 	reasonBlockStartsWithWS              = "block should not start with a whitespace"
 	reasonBlockEndsWithWS                = "block should not end with a whitespace (or comment)"
 	reasonCaseBlockTooCuddly             = "case block should end with newline at this size"
+	reasonShortDeclNotExclusive          = "short declaration should cuddle only with other short declarations"
 )
 
 // Warning strings
@@ -159,6 +160,20 @@ type Configuration struct {
 	// used for error variables to check for in the conditional.
 	// Defaults to just "err"
 	ErrorVariableNames []string
+
+	// ForceExclusiveShortDeclarations will cause an error if a short declaration
+	// (:=) cuddles with anything other than another short declaration. For example
+	//
+	// a := 2
+	// b := 3
+	//
+	// is allowed, but
+	//
+	// a := 2
+	// b = 3
+	//
+	// is not allowed. This logic overrides ForceCuddleErrCheckAndAssign among others.
+	ForceExclusiveShortDeclarations bool
 }
 
 // DefaultConfig returns default configuration
@@ -171,6 +186,7 @@ func DefaultConfig() Configuration {
 		AllowTrailingComment:             false,
 		AllowSeparatedLeadingComment:     false,
 		ForceCuddleErrCheckAndAssign:     false,
+		ForceExclusiveShortDeclarations:  false,
 		ForceCaseTrailingWhitespaceLimit: 0,
 		AllowCuddleWithCalls:             []string{"Lock", "RLock"},
 		AllowCuddleWithRHS:               []string{"Unlock", "RUnlock"},
@@ -391,6 +407,18 @@ func (p *Processor) parseBlockStatements(statements []ast.Stmt) {
 			return false
 		}
 
+		// If it's a short declaration we should not cuddle with anything else
+		// if ForceExclusiveShortDeclarations is set on; either this or the
+		// previous statement could be the short decl, so we'll find out which
+		// it was and use *that* statement's position
+		if p.config.ForceExclusiveShortDeclarations && cuddledWithLastStmt {
+			if p.isShortDecl(stmt) && !p.isShortDecl(previousStatement) {
+				p.addError(stmt.Pos(), reasonShortDeclNotExclusive)
+			} else if p.isShortDecl(previousStatement) && !p.isShortDecl(stmt) {
+				p.addError(previousStatement.Pos(), reasonShortDeclNotExclusive)
+			}
+		}
+
 		// If it's not an if statement and we're not cuddled move on. The only
 		// reason we need to keep going for if statements is to check if we
 		// should be cuddled with an error check.
@@ -888,6 +916,14 @@ func (p *Processor) findRHS(node ast.Node) []string {
 	return rhs
 }
 
+func (p *Processor) isShortDecl(node ast.Node) bool {
+	if t, ok := node.(*ast.AssignStmt); ok {
+		return t.Tok == token.DEFINE
+	}
+
+	return false
+}
+
 func (p *Processor) findBlockStmt(node ast.Node) []*ast.BlockStmt {
 	var blocks []*ast.BlockStmt
 
@@ -1034,8 +1070,17 @@ func (p *Processor) findLeadingAndTrailingWhitespaces(ident *ast.Ident, stmt, ne
 			}
 
 			// We store number of seen comment groups because we allow multiple
-			// groups with a newline between them.
-			seenCommentGroups++
+			// groups with a newline between them; but if the first one has WS
+			// before it, we're not going to count it to force an error.
+			if p.config.AllowSeparatedLeadingComment {
+				cg := p.fileSet.Position(commentGroup.Pos()).Line
+
+				if seenCommentGroups > 0 || cg == blockStartLine+1 {
+					seenCommentGroups++
+				}
+			} else {
+				seenCommentGroups++
+			}
 
 			// Support both /* multiline */ and //single line comments
 			for _, c := range commentGroup.List {
@@ -1044,14 +1089,18 @@ func (p *Processor) findLeadingAndTrailingWhitespaces(ident *ast.Ident, stmt, ne
 		}
 	}
 
-	// If we have multiple groups, add support for newline between each group.
+	// If we allow separated comments, allow for a space after each group
 	if p.config.AllowSeparatedLeadingComment {
 		if seenCommentGroups > 1 {
 			allowedLinesBeforeFirstStatement += seenCommentGroups - 1
+		} else if seenCommentGroups == 1 {
+			allowedLinesBeforeFirstStatement += 1
 		}
 	}
 
-	if p.nodeStart(firstStatement) != blockStartLine+allowedLinesBeforeFirstStatement {
+	// And now if the first statement is passed the number of allowed lines,
+	// then we had extra WS, possibly before the first comment group.
+	if p.nodeStart(firstStatement) > blockStartLine+allowedLinesBeforeFirstStatement {
 		p.addError(
 			blockStartPos,
 			reasonBlockStartsWithWS,


Test patch:
diff --git a/wsl_test.go b/wsl_test.go
index ffa361c..3dd30e9 100644
--- a/wsl_test.go
+++ b/wsl_test.go
@@ -1732,7 +1732,71 @@ func TestWithConfig(t *testing.T) {
 					*/
 					fmt.Println("Hello, World")
 				}
+
+				func () { // Comment
+					// and one more
+
+					fmt.Println("Hello, World")
+				}
+
+				func () { // Comment
+					fmt.Println("Hello, World")
+				}
+			}`),
+		},
+		{
+			description: "allow separated leading comment (negative)",
+			customConfig: &Configuration{
+				AllowSeparatedLeadingComment: true,
+			},
+			code: []byte(`package main
+
+			func main() {
+				// These blocks should generate error
+				func () {
+
+					// Comment
+
+					// Comment
+					fmt.Println("Hello, World")
+				}
+
+				func () {
+
+					fmt.Println("Hello, World")
+				}
+
+				func () {
+					// Comment
+
+
+					fmt.Println("Hello, World")
+				}
+
+				func () {
+					// Comment
+
+					// Comment
+
+
+					fmt.Println("Hello, World")
+				}
+
+				func () { // Comment
+
+					/*
+						Multiline
+					*/
+					fmt.Println("Hello, World")
+				}
 			}`),
+			expectedErrorStrings: []string{
+				reasonBlockStartsWithWS,
+				reasonBlockStartsWithWS,
+				reasonBlockStartsWithWS,
+				reasonBlockStartsWithWS,
+				reasonBlockStartsWithWS,
+			},
 		},
 		{
 			description: "only warn about cuddling errors if it's an expression above",
@@ -1828,6 +1892,35 @@ func TestWithConfig(t *testing.T) {
 				reasonOnlyOneCuddle,
 			},
 		},
+		{
+			description: "force short decls to cuddle alone",
+			code: []byte(`package main
+
+			func main() {
+				// OK
+				a := 1
+				b := 2
+
+				// should fail
+				c := 3
+				b = 2
+
+				// should fail 
+				err := DoSomething()
+				if err != nil {
+					b = 4
+				}
+			}`),
+			customConfig: &Configuration{
+				AllowAssignAndAnythingCuddle:    true,
+				ForceCuddleErrCheckAndAssign:    true,
+				ForceExclusiveShortDeclarations: true,
+			},
+			expectedErrorStrings: []string{
+				reasonShortDeclNotExclusive,
+				reasonShortDeclNotExclusive,
+			},
+		},
 	}
 
 	for _, tc := range cases {