Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify positional and keyword arguments when checking for missing arguments in docstring #4067

Merged
merged 8 commits into from
Apr 25, 2023
14 changes: 14 additions & 0 deletions crates/ruff/resources/test/fixtures/pydocstyle/D417.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,20 @@ def f(x, *args, **kwargs):
return x


def f(x, *, y, z):
"""Do something.

Args:
x: some first value

Keyword Args:
y (int): the other value
z (int): the last value

"""
return x, y, z


class Test:
def f(self, /, arg1: int) -> None:
"""
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/docstrings/google.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ pub(crate) static GOOGLE_SECTIONS: &[SectionKind] = &[
SectionKind::KeywordArguments,
SectionKind::Note,
SectionKind::Notes,
SectionKind::OtherArgs,
SectionKind::OtherArguments,
SectionKind::Return,
SectionKind::Tip,
SectionKind::Todo,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/docstrings/numpy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub(crate) static NUMPY_SECTIONS: &[SectionKind] = &[
SectionKind::Yields,
// NumPy-only
SectionKind::ExtendedSummary,
SectionKind::OtherParams,
SectionKind::OtherParameters,
SectionKind::Parameters,
SectionKind::ShortSummary,
Expand Down
9 changes: 9 additions & 0 deletions crates/ruff/src/docstrings/sections.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ pub enum SectionKind {
Methods,
Note,
Notes,
OtherArgs,
OtherArguments,
OtherParams,
OtherParameters,
Parameters,
Raises,
Expand Down Expand Up @@ -59,6 +62,9 @@ impl SectionKind {
"methods" => Some(Self::Methods),
"note" => Some(Self::Note),
"notes" => Some(Self::Notes),
"other args" => Some(Self::OtherArgs),
"other arguments" => Some(Self::OtherArguments),
"other params" => Some(Self::OtherParams),
"other parameters" => Some(Self::OtherParameters),
"parameters" => Some(Self::Parameters),
"raises" => Some(Self::Raises),
Expand Down Expand Up @@ -97,6 +103,9 @@ impl SectionKind {
Self::Methods => "Methods",
Self::Note => "Note",
Self::Notes => "Notes",
Self::OtherArgs => "Other Args",
Self::OtherArguments => "Other Arguments",
Self::OtherParams => "Other Params",
Self::OtherParameters => "Other Parameters",
Self::Parameters => "Parameters",
Self::Raises => "Raises",
Expand Down
105 changes: 66 additions & 39 deletions crates/ruff/src/rules/pydocstyle/rules/sections.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,7 @@ pub fn sections(checker: &mut Checker, docstring: &Docstring, convention: Option

match convention {
Some(Convention::Google) => {
for context in &section_contexts(&lines, SectionStyle::Google) {
google_section(checker, docstring, context);
}
parse_google_sections(checker, &lines, docstring);
}
Some(Convention::Numpy) => {
for context in &section_contexts(&lines, SectionStyle::Numpy) {
Expand All @@ -300,7 +298,9 @@ pub fn sections(checker: &mut Checker, docstring: &Docstring, convention: Option
if numpy_sections.iter().any(|context| {
matches!(
context.kind,
SectionKind::Parameters | SectionKind::OtherParameters
SectionKind::Parameters
| SectionKind::OtherParams
| SectionKind::OtherParameters
)
}) {
for context in &numpy_sections {
Expand All @@ -309,23 +309,26 @@ pub fn sections(checker: &mut Checker, docstring: &Docstring, convention: Option
return;
}

// If the docstring contains `Args:` or `Arguments:`, use the Google convention.
// If the docstring contains any argument specifier, use the Google convention.
let google_sections = section_contexts(&lines, SectionStyle::Google);
if google_sections
.iter()
.any(|context| matches!(context.kind, SectionKind::Arguments | SectionKind::Args))
{
for context in &google_sections {
google_section(checker, docstring, context);
}
if google_sections.iter().any(|context| {
matches!(
context.kind,
SectionKind::Args
| SectionKind::Arguments
| SectionKind::KeywordArgs
| SectionKind::KeywordArguments
| SectionKind::OtherArgs
| SectionKind::OtherArguments
)
}) {
parse_google_sections(checker, &lines, docstring);
return;
}

// Otherwise, use whichever convention matched more sections.
if google_sections.len() > numpy_sections.len() {
for context in &google_sections {
google_section(checker, docstring, context);
}
parse_google_sections(checker, &lines, docstring);
} else {
for context in &numpy_sections {
numpy_section(checker, docstring, context);
Expand Down Expand Up @@ -790,7 +793,7 @@ fn common_section(checker: &mut Checker, docstring: &Docstring, context: &Sectio
blanks_and_section_underline(checker, docstring, context);
}

fn missing_args(checker: &mut Checker, docstring: &Docstring, docstrings_args: &FxHashSet<&str>) {
fn missing_args(checker: &mut Checker, docstring: &Docstring, docstrings_args: &FxHashSet<String>) {
let (
DefinitionKind::Function(parent)
| DefinitionKind::NestedFunction(parent)
Expand Down Expand Up @@ -824,30 +827,30 @@ fn missing_args(checker: &mut Checker, docstring: &Docstring, docstrings_args: &
),
)
{
let arg_name = arg.node.arg.as_str();
if !arg_name.starts_with('_') && !docstrings_args.contains(&arg_name) {
let arg_name = &arg.node.arg;
if !arg_name.starts_with('_') && !docstrings_args.contains(arg_name) {
missing_arg_names.insert(arg_name.to_string());
}
}

// Check specifically for `vararg` and `kwarg`, which can be prefixed with a
// single or double star, respectively.
if let Some(arg) = &arguments.vararg {
let arg_name = arg.node.arg.as_str();
let arg_name = &arg.node.arg;
let starred_arg_name = format!("*{arg_name}");
if !arg_name.starts_with('_')
&& !docstrings_args.contains(&arg_name)
&& !docstrings_args.contains(&starred_arg_name.as_str())
&& !docstrings_args.contains(arg_name)
&& !docstrings_args.contains(&starred_arg_name)
{
missing_arg_names.insert(starred_arg_name);
}
}
if let Some(arg) = &arguments.kwarg {
let arg_name = arg.node.arg.as_str();
let arg_name = &arg.node.arg;
let starred_arg_name = format!("**{arg_name}");
if !arg_name.starts_with('_')
&& !docstrings_args.contains(&arg_name)
&& !docstrings_args.contains(&starred_arg_name.as_str())
&& !docstrings_args.contains(arg_name)
&& !docstrings_args.contains(&starred_arg_name)
{
missing_arg_names.insert(starred_arg_name);
}
Expand All @@ -863,13 +866,13 @@ fn missing_args(checker: &mut Checker, docstring: &Docstring, docstrings_args: &
}

// See: `GOOGLE_ARGS_REGEX` in `pydocstyle/checker.py`.
static GOOGLE_ARGS_REGEX: Lazy<Regex> =
Lazy::new(|| Regex::new(r"^\s*(\*?\*?\w+)\s*(\(.*?\))?\s*:(\r\n|\n)?\s*.+").unwrap());
static GOOGLE_ARGS_REGEX: Lazy<Regex> = Lazy::new(|| {
Regex::new(r"^\s*(?P<arg_name>\*?\*?\w+)\s*(\(.*?\))?\s*:(\r\n|\n)?\s*.+").unwrap()
evanrittenhouse marked this conversation as resolved.
Show resolved Hide resolved
});

fn args_section(checker: &mut Checker, docstring: &Docstring, context: &SectionContext) {
fn args_section(context: &SectionContext) -> FxHashSet<String> {
if context.following_lines.is_empty() {
missing_args(checker, docstring, &FxHashSet::default());
return;
return FxHashSet::default();
}

// Normalize leading whitespace, by removing any lines with less indentation
Expand Down Expand Up @@ -908,17 +911,20 @@ fn args_section(checker: &mut Checker, docstring: &Docstring, context: &SectionC
matches.push(captures);
}
}
let docstrings_args = matches
.iter()
.filter_map(|captures| captures.get(1).map(|arg_name| arg_name.as_str()))
.collect();

missing_args(checker, docstring, &docstrings_args);
matches
.iter()
.filter_map(|captures| {
captures
.name("arg_name")
.map(|arg_name| arg_name.as_str().to_owned())
})
.collect::<FxHashSet<String>>()
}

fn parameters_section(checker: &mut Checker, docstring: &Docstring, context: &SectionContext) {
// Collect the list of arguments documented in the docstring.
let mut docstring_args: FxHashSet<&str> = FxHashSet::default();
let mut docstring_args: FxHashSet<String> = FxHashSet::default();
let section_level_indent = whitespace::leading_space(context.line);

// Join line continuations, then resplit by line.
Expand All @@ -941,7 +947,7 @@ fn parameters_section(checker: &mut Checker, docstring: &Docstring, context: &Se
// Notably, NumPy lets you put multiple parameters of the same type on the same
// line.
for parameter in parameters.split(',') {
docstring_args.insert(parameter.trim());
docstring_args.insert(parameter.trim().to_owned());
}
}

Expand Down Expand Up @@ -1044,10 +1050,31 @@ fn google_section(checker: &mut Checker, docstring: &Docstring, context: &Sectio
checker.diagnostics.push(diagnostic);
}
}
}

if checker.settings.rules.enabled(Rule::UndocumentedParam) {
if matches!(context.kind, SectionKind::Args | SectionKind::Arguments) {
args_section(checker, docstring, context);
fn parse_google_sections(checker: &mut Checker, lines: &[&str], docstring: &Docstring) {
let mut documented_args: FxHashSet<String> = FxHashSet::default();
let section_contexts = &section_contexts(lines, SectionStyle::Google);
for section_context in section_contexts {
// Checks occur at the section level. Since two sections (args/keyword args and their
// variants) can list arguments, we need to unify the sets of arguments mentioned in both
// then check for missing arguments at the end of the section check.
if matches!(
section_context.kind,
SectionKind::Args
| SectionKind::Arguments
| SectionKind::KeywordArgs
| SectionKind::KeywordArguments
| SectionKind::OtherArgs
| SectionKind::OtherArguments
) {
documented_args.extend(args_section(section_context));
}

google_section(checker, docstring, section_context);
}

if checker.settings.rules.enabled(Rule::UndocumentedParam) {
missing_args(checker, docstring, &documented_args);
evanrittenhouse marked this conversation as resolved.
Show resolved Hide resolved
}
}