feat: Review Feedback (#12)

* Minimum function length flag for linting function documentation
in doculint linter.
* Granular required header fields that must exist before the package
keyword, configurable via flags in the header linter.
* Required copyright linter that ensures a configurable (via flags)
copyright message exists on line 1 of each .go file.

README.md

@@ -22,6 +22,35 @@ go install ./cmd/lintroller
 
 # Navigate to a repository you'd like to run the lintroller against.
 go vet -vettool $(which lintroller) ./...
+
+# Example of passing flags:
+go vet -vettool $(which lintroller) -header.fields description,gotchas ./...
+```
+
+An alternative way to run the tool which is easier for rapid development is to build and then pass the absolute path of
+the binary to `go vet -vettool`:
+
+```shell
+# In the root of this repository:
+make build
+
+# In the root of a repository you're testing against:
+go vet -vettool ~/go/src/github.com/getoutreach/lintroller/bin/lintroller ./...
+```
+
+The reason this is easier for rapid development is because these two steps can be done in separate terminal windows/panes/
+tabs and it removes the annoyance of dealing with cached binaries that come along with `go install`.
+
+## Singular Linters and Flags
+
+To get information regarding singular linters and their flags you can run the following command(s) (after building):
+
+```shell
+# Provides a list of linters defined within lintroller.
+./bin/lintroller help
+
+# Shows the flags, descriptions, and defaults for an individual linter.
+./bin/lintroller help <linter>
 ```
 <!--- EndBlock(custom) -->
 

cmd/lintroller/main.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"github.com/getoutreach/lintroller/internal/copyright"
 	"github.com/getoutreach/lintroller/internal/doculint"
 	"github.com/getoutreach/lintroller/internal/header"
 	"golang.org/x/tools/go/analysis/unitchecker"
@@ -10,6 +11,7 @@ func main() {
 	unitchecker.Main(
 		&doculint.Analyzer,
 		&header.Analyzer,
+		&copyright.Analyzer,
 		// Add more *analysis.Analyzer's here.
 	)
 }

go.mod

@@ -2,4 +2,7 @@ module github.com/getoutreach/lintroller
 
 go 1.15
 
-require golang.org/x/tools v0.1.2
+require (
+	github.com/pkg/errors v0.9.1
+	golang.org/x/tools v0.1.2
+)

internal/copyright/copyright.go

@@ -0,0 +1,121 @@
+package copyright
+
+import (
+	"regexp"
+	"strings"
+
+	"github.com/pkg/errors"
+	"golang.org/x/tools/go/analysis"
+)
+
+// Here is an example regular expression that can be used to test this linter:
+// ^Copyright 20[2-9][0-9] Outreach Corporation\.  All Rights Reserved\.$
+
+// doc defines the help text for the copyright linter.
+const doc = `Ensures each .go file has a comment at the top of the file containing the 
+copyright string requested via flags.`
+
+// Analyzer exports the copyright analyzer (linter).
+var Analyzer = analysis.Analyzer{
+	Name: "copyright",
+	Doc:  doc,
+	Run:  copyright,
+}
+
+// Variable block to keep track of flags whose values are collected at runtime. See the
+// init function that immediately proceeds this block to see more.
+var (
+	// copyrightString is a variable that gets collected via flags. This variable contains
+	// the copyright string required at the top of each .go file.
+	copyrightString string
+
+	// regularExpression is a variable that gets collected via flags. This variable denotes
+	// whether or not the copyrightString passed via the copyright flag is a regular
+	// expression or just a normal string.
+	regularExpression bool
+)
+
+func init() { //nolint:gochecknoinits
+	Analyzer.Flags.StringVar(&copyrightString, "copyright", "", "the copyright string required at the top of each .go file. if empty this linter is a no-op")
+	Analyzer.Flags.BoolVar(&regularExpression, "regex", false, "denotes whether or not the copyright string is a regular expression")
+}
+
+// comparer is a convience type used to conditionally compare using either a string or a
+// compiled regular expression based off of the existence of the regular expression.
+type comparer struct {
+	copyrightString string
+	copyrightRegex  *regexp.Regexp
+}
+
+// compare uses the struct fields stored in the receiver to conditionally compare a given
+// string value.
+func (c *comparer) compare(value string) bool {
+	// If the copyright regular expression is non-nil, use that to
+	// compare.
+	if c.copyrightRegex != nil {
+		return c.copyrightRegex.MatchString(value)
+	}
+
+	// Else, use the copyright string to compare.
+	if c.copyrightString == value {
+		return true
+	}
+	return false
+}
+
+// copyright is the function that gets passed to the Analyzer which runs the actual
+// analysis for the copyright linter on a set of files.
+func copyright(pass *analysis.Pass) (interface{}, error) { //nolint:funlen
+	if copyrightString == "" {
+		return nil, nil
+	}
+
+	c := comparer{
+		copyrightString: copyrightString,
+	}
+
+	if regularExpression {
+		reCopyright, err := regexp.Compile(c.copyrightString)
+		if err != nil {
+			return nil, errors.Wrap(err, "compile copyright regular expression")
+		}
+		c.copyrightRegex = reCopyright
+	}
+
+	for _, file := range pass.Files {
+		fp := pass.Fset.PositionFor(file.Package, false).Filename
+
+		// Variable to keep track of whether or not the copyright string was found at the
+		// top of the current file.
+		var foundCopyright bool
+
+		for _, commentGroup := range file.Comments {
+			if pass.Fset.PositionFor(commentGroup.Pos(), false).Line != 1 {
+				// The copyright comment needs to be on line 1. Ignore all other comments.
+				continue
+			}
+
+			// Set the value of the foundCopyright to the comparison of this comment's text
+			// to the stored copyrightString value or the regular expression compiled from
+			// it.
+			foundCopyright = c.compare(strings.TrimSpace(commentGroup.Text()))
+
+			// We can safely break here because if we got here, regardless on the outcome of
+			// the previous statement, we know this is the only comment that matters because
+			// it is on line one. This can be verified by the conditional at the top of the
+			// current loop.
+			break
+		}
+
+		if !foundCopyright {
+			matchType := "string"
+			if regularExpression {
+				matchType = "regular expression"
+			}
+
+			pass.Reportf(0, "file \"%s\" does not contain the required copyright %s [%s] (sans-brackets) as a comment on line 1", fp, matchType, copyrightString)
+		}
+	}
+
+	return nil, nil
+}

internal/doculint/doculint.go

@@ -12,13 +12,30 @@ import (
 	"golang.org/x/tools/go/analysis"
 )
 
+// doc defines the help text for the doculint linter.
+const doc = `Checks for proper function, type, package, constant, and string and numeric 
+literal documentation in accordance with godoc standards.`
+
 // Analyzer exports the doculint analyzer (linter).
 var Analyzer = analysis.Analyzer{
 	Name: "doculint",
-	Doc:  "checks for proper function, type, package, constant, and string and numeric literal documentation",
+	Doc:  doc,
 	Run:  doculint,
 }
 
+// Variable block to keep track of flags whose values are collected at runtime. See the
+// init function that immediately proceeds this block to see more.
+var (
+	// minFunLen is a variable that gets collected via flags. This variable contains a
+	// the minimum function length that doculint will report on if said function has no
+	// related documentation.
+	minFunLen int
+)
+
+func init() { //nolint:gochecknoinits
+	Analyzer.Flags.IntVar(&minFunLen, "minFunLen", 10, "the minimum function length that doculint will report on if said function has no related documentation")
+}
+
 // doculint is the function that gets passed to the Analyzer which runs the actual
 // analysis for the doculint linter on a set of files.
 func doculint(pass *analysis.Pass) (interface{}, error) { //nolint:funlen
@@ -68,8 +85,18 @@ func doculint(pass *analysis.Pass) (interface{}, error) { //nolint:funlen
 					return true
 				}
 
-				// Run through function declaration validation rules.
-				validateFuncDecl(pass, expr)
+				start := pass.Fset.PositionFor(expr.Pos(), false).Line
+				end := pass.Fset.PositionFor(expr.End(), false).Line
+
+				// The reason a 1 is added is to account for single-line functions (edge case).
+				// This also doesn't affect non-single line functions, it will just account for
+				// the trailing } which is what most people would expect anyways when providing
+				// a minimum function length to validate against.
+				if (end - start + 1) >= minFunLen {
+					// Run through function declaration validation rules if the minimum function
+					// length is met or exceeded.
+					validateFuncDecl(pass, expr)
+				}
 			case *ast.GenDecl:
 				// Run through general declaration validation rules, currently these
 				// only apply to constants, type, and variable declarations, as you

internal/header/header.go

@@ -8,16 +8,61 @@ import (
 	"golang.org/x/tools/go/analysis"
 )
 
+// doc defines the help text for the header linter.
+const doc = `The header linter ensures each .go file has a header comment section
+defined before the package keyword that matches the fields defined via the -fields
+flag.
+
+As an example, say the following flag was passed to the linter:
+
+	-fields=Author(s),Description,Gotchas
+
+This would mean the linter would pass on files that resemble the following:
+
+	// Authors(s): <value>
+	// Description: <multi-line
+	// value>
+	// Gotchas: <value>
+
+	package foo
+
+The <value> portion of each field can extend to the next line, as shown in the
+value for the description field above; however, there can't be a break in the
+comment group for the header, as follows:
+
+	// Authors(s): <value>
+
+	// Description: <value>
+	// Gotchas: <value>
+
+	package foo`
+
 // Analyzer exports the doculint analyzer (linter).
 var Analyzer = analysis.Analyzer{
 	Name: "header",
-	Doc:  "ensures each .go file has a header that explains the purpose of the file and any notable design decisions",
+	Doc:  doc,
 	Run:  header,
 }
 
+// Variable block to keep track of flags whose values are collected at runtime. See the
+// init function that immediately proceeds this block to see more.
+var (
+	// rawFields is a variable that gets collected via flags. This variable contains a
+	// comma-separated list of fields required to be filled out within the header of a
+	// file.
+	rawFields string
+)
+
+func init() { //nolint:gochecknoinits
+	Analyzer.Flags.StringVar(&rawFields, "fields", "Description", "comma-separated list of fields required to be filled out in the header")
+}
+
 // header is the function that gets passed to the Analyzer which runs the actual
 // analysis for the header linter on a set of files.
-func header(pass *analysis.Pass) (interface{}, error) {
+func header(pass *analysis.Pass) (interface{}, error) { //nolint:funlen
+	fields := strings.Split(rawFields, ",")
+	validFields := make(map[string]bool, len(fields))
+
 	for _, file := range pass.Files {
 		if pass.Pkg.Name() == common.PackageMain {
 			// Ignore the main package, there should really one ever be one file in the
@@ -27,28 +72,68 @@ func header(pass *analysis.Pass) (interface{}, error) {
 			continue
 		}
 
-		var foundHeaderComment bool
-		for _, comment := range file.Comments {
-			line := pass.Fset.PositionFor(comment.Pos(), false).Line
+		// Reset the validFields variable and assume all fields are valid by default
+		// for each file in this pass (package).
+		for i := range fields {
+			validFields[fields[i]] = false
+		}
+
+		// Note the package keyword line. All of these header comments must exist before
+		// this line number.
+		packageKeywordLine := pass.Fset.PositionFor(file.Package, false).Line
+
+		for _, commentGroup := range file.Comments {
+			line := pass.Fset.PositionFor(commentGroup.Pos(), false).Line
+			if line >= packageKeywordLine {
+				// Ignore comments past the line that the package keyword is on. These header
+				// fields are required to exist before that.
+				continue
+			}
+
+			var numFound int
+
+			// Look to see if all of the fields are found in the format we expect them to be
+			// in (sans-quotes):
+			// "<field>: "
+			// by a simple strings.Contains check in the entire text of the comment group. If
+			// we end up finding all fields we will do further validation.
+			for i := range fields {
+				if strings.Contains(commentGroup.Text(), fmt.Sprintf("%s: ", fields[i])) {
+					numFound++
+				}
+			}
+
+			// All fields are found, do further validation.
+			if len(fields) == numFound {
+				for _, comment := range commentGroup.List {
+					for i := range fields {
+						cleanComment := strings.TrimSpace(strings.TrimPrefix(comment.Text, "//"))
+						prefix := fmt.Sprintf("%s: ", fields[i])
 
-			// Check to see if the comment starts at the first line of the file.
-			if line == 1 {
-				if strings.HasPrefix(comment.Text(), fmt.Sprintf("Package %s", pass.Pkg.Name())) {
-					// This is the package comment, which is not a header comment. Break
-					// and report that this file does not have a header comment.
-					break
+						if strings.HasPrefix(cleanComment, prefix) {
+							if len(strings.TrimPrefix(cleanComment, prefix)) > 0 {
+								// If the current comment line has a field prefix we're looking for and
+								// data proceeding the colon and space after the colon, we will mark the
+								// field as valid.
+								validFields[fields[i]] = true
+							}
+						}
+					}
 				}
 
-				foundHeaderComment = true
+				// We found a comment block containing all fields, we don't need to search any further.
 				break
 			}
 		}
 
-		if !foundHeaderComment {
-			// Get current filepath for reporting.
-			fp := pass.Fset.PositionFor(file.Package, false).Filename
+		// Get current filepath for potential reporting.
+		fp := pass.Fset.PositionFor(file.Package, false).Filename
 
-			pass.Reportf(0, "file \"%s\" does not contain a header that explains the purpose of the file and any notable design decisions", fp)
+		for field, valid := range validFields {
+			if !valid {
+				// Required field not found, report it.
+				pass.Reportf(0, "file \"%s\" does not contain the required header key \"%s\" and corresponding value existing before the package keyword", fp, field)
+			}
 		}
 	}