diff --git a/warn/warn_docstring.go b/warn/warn_docstring.go index 92c92dc19..4e12c90e7 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)) } diff --git a/warn/warn_docstring_test.go b/warn/warn_docstring_test.go index 22e4dd6e4..f0b2d9011 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) }