Update the functions docstring warning (#543)

bazelbuild · Feb 12, 2019 · fbccc74 · fbccc74
1 parent a17901e
commit fbccc74
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 20 deletions.
diff --git 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))
 		}

diff --git 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)
 }