From 5379bdbe7c11abdb2136b51164e73a1bcb3eb26c Mon Sep 17 00:00:00 2001
From: "Iskander (Alex) Sharipov" <quasilyte@gmail.com>
Date: Sat, 2 Mar 2019 05:48:40 +0300
Subject: [PATCH] checkers: follow-up for #814 (#818)

- Using ast file (fPos) is not needed.
  It's possible to get Pos line by using a node and FileSet only.

- Trailing period in the Summary is redundant.
  We need to write tests for doc objects validation.

- Using plural forms in checker names is uncommon.
  s/dupImports/dupImport/

- Before/After examples should be as minimal, as possible.
  They are not required to be a complete Go program.
  Simplified them a bit.

- Code that emits a warning and builds up a message
  should be moved to "warn" function.
  This is a convention.

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>
---
 checkers/dupImports_checker.go                | 53 +++++++------------
 .../negative_tests.go                         |  0
 .../positive_tests.go                         |  0
 3 files changed, 20 insertions(+), 33 deletions(-)
 rename checkers/testdata/{dupImports => dupImport}/negative_tests.go (100%)
 rename checkers/testdata/{dupImports => dupImport}/positive_tests.go (100%)

diff --git a/checkers/dupImports_checker.go b/checkers/dupImports_checker.go
index 7a6c837c6..d531413a1 100644
--- a/checkers/dupImports_checker.go
+++ b/checkers/dupImports_checker.go
@@ -9,28 +9,18 @@ import (
 
 func init() {
 	var info lintpack.CheckerInfo
-	info.Name = "dupImports"
+	info.Name = "dupImport"
 	info.Tags = []string{"style", "experimental"}
-	info.Summary = "Detects multiple imports of the same package under different aliases."
+	info.Summary = "Detects multiple imports of the same package under different aliases"
 	info.Before = `
 import (
 	"fmt"
-	priting "fmt"
-)
-
-func helloWorld() {
-	fmt.Println("Hello")
-	printing.Println("World")
-}`
+	priting "fmt" // Imported the second time
+)`
 	info.After = `
 import(
 	"fmt"
-)
-
-func helloWorld() {
-	fmt.Println("Hello")
-	fmt.Println("World")
-}`
+)`
 
 	collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
 		return &dupImportChecker{ctx: ctx}
@@ -48,29 +38,26 @@ func (c *dupImportChecker) WalkFile(f *ast.File) {
 		imports[pkg] = append(imports[pkg], importDcl)
 	}
 
-	fPos := c.ctx.FileSet.File(f.Pos())
-	if fPos == nil {
-		// Bail out if we can't, for any reason, determine
-		// the source file we are dealing with.
-		return
-	}
 	for _, importList := range imports {
 		if len(importList) == 1 {
 			continue
 		}
+		c.warn(importList)
+	}
+}
 
-		msg := fmt.Sprintf("package is imported %d times under different aliases on lines", len(importList))
-		for idx, importDcl := range importList {
-			switch {
-			case idx == len(importList)-1:
-				msg += " and"
-			case idx > 0:
-				msg += ","
-			}
-			msg += fmt.Sprintf(" %d", fPos.Line(importDcl.Pos()))
-		}
-		for _, importDcl := range importList {
-			c.ctx.Warn(importDcl, msg)
+func (c *dupImportChecker) warn(importList []*ast.ImportSpec) {
+	msg := fmt.Sprintf("package is imported %d times under different aliases on lines", len(importList))
+	for idx, importDcl := range importList {
+		switch {
+		case idx == len(importList)-1:
+			msg += " and"
+		case idx > 0:
+			msg += ","
 		}
+		msg += fmt.Sprintf(" %d", c.ctx.FileSet.Position(importDcl.Pos()).Line)
+	}
+	for _, importDcl := range importList {
+		c.ctx.Warn(importDcl, msg)
 	}
 }
diff --git a/checkers/testdata/dupImports/negative_tests.go b/checkers/testdata/dupImport/negative_tests.go
similarity index 100%
rename from checkers/testdata/dupImports/negative_tests.go
rename to checkers/testdata/dupImport/negative_tests.go
diff --git a/checkers/testdata/dupImports/positive_tests.go b/checkers/testdata/dupImport/positive_tests.go
similarity index 100%
rename from checkers/testdata/dupImports/positive_tests.go
rename to checkers/testdata/dupImport/positive_tests.go