Repository: bazelbuild/buildtools
Pull request #543 (2019-02-12 18:22:51)

Update the functions docstring warning
If a function is private or not long enough, don't force its docstring to have arguments or the return value documented.

However if at least some arguments are documented, check that all of them are.


Patch:
diff --git a/warn/warn_docstring.go b/warn/warn_docstring.go
index 92c92dc..4e12c90 100644
--- a/warn/warn_docstring.go
+++ b/warn/warn_docstring.go
@@ -211,9 +211,12 @@ func functionDocstringWarning(f *build.File, fix bool) []*Finding {
 			continue
 		}
 
+		// A docstring is required for public functions if they are long enough (at least 5 statements)
+		isDocstringRequired := !strings.HasPrefix(def.Name, "_") && stmtsCount(def.Body) >= FunctionLengthDocstringThreshold
+
 		doc, ok := getDocstring(def.Body)
 		if !ok {
-			if !strings.HasPrefix(def.Name, "_") && stmtsCount(def.Body) >= FunctionLengthDocstringThreshold {
+			if isDocstringRequired {
 				// Public functions that are not too short should have a docstring
 				start, end := stmt.Span()
 				findings = append(findings, makeFinding(f, start, end, "function-docstring",
@@ -237,26 +240,33 @@ func functionDocstringWarning(f *build.File, fix bool) []*Finding {
 				`Prefer 'Args:' to 'Arguments:' when documenting function arguments.`, true, nil))
 		}
 
-		paramNames := make(map[string]bool)
-		for _, param := range def.Params {
-			name := getParamName(param)
-			paramNames[name] = true
-			if _, ok := info.args[name]; !ok {
-				findings = append(findings, makeFinding(f, start, end, "function-docstring",
-					fmt.Sprintf(`Argument "%s" is not documented.`, name), true, nil))
+		// If the docstring is required or there are any arguments described, check for their integrity.
+		if isDocstringRequired || len(info.args) > 0 {
+
+			// Check whether all arguments are documented.
+			paramNames := make(map[string]bool)
+			for _, param := range def.Params {
+				name := getParamName(param)
+				paramNames[name] = true
+				if _, ok := info.args[name]; !ok {
+					findings = append(findings, makeFinding(f, start, end, "function-docstring",
+						fmt.Sprintf(`Argument "%s" is not documented.`, name), true, nil))
+				}
 			}
-		}
 
-		for name, pos := range info.args {
-			if !paramNames[name] {
-				posEnd := pos
-				posEnd.LineRune += len(name)
-				findings = append(findings, makeFinding(f, pos, posEnd, "function-docstring",
-					fmt.Sprintf(`Argument "%s" is documented but doesn't exist in the function signature.`, name), true, nil))
+			// Check whether all documented arguments actually exist in the function signature.
+			for name, pos := range info.args {
+				if !paramNames[name] {
+					posEnd := pos
+					posEnd.LineRune += len(name)
+					findings = append(findings, makeFinding(f, pos, posEnd, "function-docstring",
+						fmt.Sprintf(`Argument "%s" is documented but doesn't exist in the function signature.`, name), true, nil))
+				}
 			}
 		}
 
-		if hasReturnValues(def) && !info.returns {
+		// Check whether the return value is documented
+		if isDocstringRequired && hasReturnValues(def) && !info.returns {
 			findings = append(findings, makeFinding(f, start, end, "function-docstring",
 				fmt.Sprintf(`Return value of "%s" is not documented.`, def.Name), true, nil))
 		}


Test patch:
diff --git a/warn/warn_docstring_test.go b/warn/warn_docstring_test.go
index 22e4dd6..f0b2d90 100644
--- a/warn/warn_docstring_test.go
+++ b/warn/warn_docstring_test.go
@@ -68,10 +68,22 @@ def f(x):
 def f(x):
    """Short function with a docstring"""
    return x
+`,
+		[]string{},
+		scopeEverywhere)
+
+	checkFindings(t, "function-docstring", `
+def f(x, y):
+   """Short function with a docstring
+
+   Arguments:
+     x: smth
+   """
+   return x + y
 `,
 		[]string{
-			`:2: Argument "x" is not documented.`,
-			`:2: Return value of "f" is not documented.`,
+			`2: Argument "y" is not documented.`,
+			`4: Prefer 'Args:' to 'Arguments:' when documenting function arguments.`,
 		},
 		scopeEverywhere)
 
@@ -114,8 +126,26 @@ def _f(x):
 `,
 		[]string{
 			`:2: The docstring for the function "_f" should start with a one-line summary.`,
-			`:2: Argument "x" is not documented.`,
-			`:2: Return value of "_f" is not documented.`,
+		},
+		scopeEverywhere)
+
+	checkFindings(t, "function-docstring", `
+def _f(x, y):
+   """Long private function
+   
+   Args:
+     x: something
+     z: something
+   """
+   x *= 2
+   x /= 3
+   x -= 4
+   x %= 5
+   return x
+`,
+		[]string{
+			`:2: Argument "y" is not documented.`,
+			`:6: Argument "z" is documented but doesn't exist in the function signature.`,
 		},
 		scopeEverywhere)
 }