From 6a8a2280f8910d4268380400d7888cb8d72b4296 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Mazzucotelli?= Date: Sun, 27 Aug 2023 19:30:13 +0200 Subject: [PATCH] refactor: Be more strict when parsing sections in Google docstrings Issue #204: https://github.com/mkdocstrings/griffe/issues/204 --- docs/docstrings.md | 50 +++++++++++- src/griffe/docstrings/google.py | 118 ++++++++++++--------------- src/griffe/docstrings/utils.py | 14 +++- tests/conftest.py | 4 - tests/test_docstrings/helpers.py | 3 +- tests/test_docstrings/test_google.py | 94 +++++++++++---------- 6 files changed, 162 insertions(+), 121 deletions(-) diff --git a/docs/docstrings.md b/docs/docstrings.md index b37a3079..b19215a8 100644 --- a/docs/docstrings.md +++ b/docs/docstrings.md @@ -46,9 +46,55 @@ section identifier: optional section title All sections identifiers are case-insensitive. All sections support multiple lines in descriptions, -as well as blank lines. +as well as blank lines. The first line must not be blank. +Each section must be separated from contents above by a blank line. -Some sections also support documenting multiple items. +❌ This is **invalid** and will be parsed as regular markup: + +```python +Some text. +Note: # (1)! + Some information. + + Blank lines allowed. +``` + +1. Missing blank line above. + +❌ This is **invalid** and will be parsed as regular markup: + +```python +Some text. + +Note: # (1)! + + Some information. + + Blank lines allowed. +``` + +1. Extraneous blank line below. + +✅ This is **valid** and will parsed as a text section followed by a note admonition: + +```python +Some text. + +Note: + Some information. + + Blank lines allowed. +``` + +Find out possibly invalid section syntax by grepping for "reasons" in Griffe debug logs: + +```bash +griffe dump -Ldebug -o/dev/null \ + -fdgoogle -D '{"strict_sections": true}' \ + your_package 2>&1 | grep reasons +``` + +Some sections support documenting multiple items (attributes, parameters, etc.). When multiple items are supported, each item description can use multiple lines, and continuation lines must be indented once more so that the parser is able to differentiate items. diff --git a/src/griffe/docstrings/google.py b/src/griffe/docstrings/google.py index b30bef5b..7ca55e18 100644 --- a/src/griffe/docstrings/google.py +++ b/src/griffe/docstrings/google.py @@ -37,6 +37,7 @@ ) from griffe.docstrings.utils import parse_annotation, warning from griffe.expressions import ExprName +from griffe.logger import LogLevel if TYPE_CHECKING: from typing import Any, Literal, Pattern @@ -245,12 +246,7 @@ def _read_parameters_section( **options: Any, ) -> tuple[DocstringSectionParameters | None, int]: parameters, new_offset = _read_parameters(docstring, offset=offset, **options) - - if parameters: - return DocstringSectionParameters(parameters), new_offset - - _warn(docstring, new_offset, f"Empty parameters section at line {offset}") - return None, new_offset + return DocstringSectionParameters(parameters), new_offset def _read_other_parameters_section( @@ -261,12 +257,7 @@ def _read_other_parameters_section( **options: Any, ) -> tuple[DocstringSectionOtherParameters | None, int]: parameters, new_offset = _read_parameters(docstring, offset=offset, warn_unknown_params=False, **options) - - if parameters: - return DocstringSectionOtherParameters(parameters), new_offset - - _warn(docstring, new_offset, f"Empty other parameters section at line {offset}") - return None, new_offset + return DocstringSectionOtherParameters(parameters), new_offset def _read_attributes_section( @@ -302,11 +293,7 @@ def _read_attributes_section( attributes.append(DocstringAttribute(name=name, annotation=annotation, description=description)) - if attributes: - return DocstringSectionAttributes(attributes), new_offset - - _warn(docstring, new_offset, f"Empty attributes section at line {offset}") - return None, new_offset + return DocstringSectionAttributes(attributes), new_offset def _read_functions_section( @@ -337,11 +324,7 @@ def _read_functions_section( functions.append(DocstringFunction(name=name, annotation=signature, description=description)) - if functions: - return DocstringSectionFunctions(functions), new_offset - - _warn(docstring, new_offset, f"Empty functions/methods section at line {offset}") - return None, new_offset + return DocstringSectionFunctions(functions), new_offset def _read_classes_section( @@ -372,11 +355,7 @@ def _read_classes_section( classes.append(DocstringClass(name=name, annotation=signature, description=description)) - if classes: - return DocstringSectionClasses(classes), new_offset - - _warn(docstring, new_offset, f"Empty classes section at line {offset}") - return None, new_offset + return DocstringSectionClasses(classes), new_offset def _read_modules_section( @@ -397,11 +376,7 @@ def _read_modules_section( description = "\n".join([description.lstrip(), *module_lines[1:]]).rstrip("\n") modules.append(DocstringModule(name=name, description=description)) - if modules: - return DocstringSectionModules(modules), new_offset - - _warn(docstring, new_offset, f"Empty modules section at line {offset}") - return None, new_offset + return DocstringSectionModules(modules), new_offset def _read_raises_section( @@ -425,11 +400,7 @@ def _read_raises_section( annotation = parse_annotation(annotation, docstring) exceptions.append(DocstringRaise(annotation=annotation, description=description)) - if exceptions: - return DocstringSectionRaises(exceptions), new_offset - - _warn(docstring, new_offset, f"Empty exceptions section at line {offset}") - return None, new_offset + return DocstringSectionRaises(exceptions), new_offset def _read_warns_section( @@ -450,11 +421,7 @@ def _read_warns_section( description = "\n".join([description.lstrip(), *warning_lines[1:]]).rstrip("\n") warns.append(DocstringWarn(annotation=annotation, description=description)) - if warns: - return DocstringSectionWarns(warns), new_offset - - _warn(docstring, new_offset, f"Empty warns section at line {offset}") - return None, new_offset + return DocstringSectionWarns(warns), new_offset def _read_returns_section( @@ -516,11 +483,7 @@ def _read_returns_section( returns.append(DocstringReturn(name=name or "", annotation=annotation, description=description)) - if returns: - return DocstringSectionReturns(returns), new_offset - - _warn(docstring, new_offset, f"Empty returns section at line {offset}") - return None, new_offset + return DocstringSectionReturns(returns), new_offset def _read_yields_section( @@ -567,11 +530,7 @@ def _read_yields_section( yields.append(DocstringYield(name=name or "", annotation=annotation, description=description)) - if yields: - return DocstringSectionYields(yields), new_offset - - _warn(docstring, new_offset, f"Empty yields section at line {offset}") - return None, new_offset + return DocstringSectionYields(yields), new_offset def _read_receives_section( @@ -614,11 +573,7 @@ def _read_receives_section( receives.append(DocstringReceive(name=name or "", annotation=annotation, description=description)) - if receives: - return DocstringSectionReceives(receives), new_offset - - _warn(docstring, new_offset, f"Empty receives section at line {offset}") - return None, new_offset + return DocstringSectionReceives(receives), new_offset def _read_examples_section( @@ -677,11 +632,7 @@ def _read_examples_section( elif current_example: sub_sections.append((DocstringSectionKind.examples, "\n".join(current_example))) - if sub_sections: - return DocstringSectionExamples(sub_sections), new_offset - - _warn(docstring, new_offset, f"Empty examples section at line {offset}") - return None, new_offset + return DocstringSectionExamples(sub_sections), new_offset def _read_deprecated_section( @@ -692,11 +643,6 @@ def _read_deprecated_section( ) -> tuple[DocstringSectionDeprecated | None, int]: text, new_offset = _read_block(docstring, offset=offset, **options) - # early exit if there is no text in the yield section - if not text: - _warn(docstring, new_offset, f"Empty deprecated section at line {offset}") - return None, new_offset - # check the presence of a name and description, separated by a semi-colon try: version, text = text.split(":", 1) @@ -733,6 +679,8 @@ def _is_empty_line(line: str) -> bool: DocstringSectionKind.deprecated: _read_deprecated_section, } +_sentinel = object() + def parse( docstring: Docstring, @@ -800,7 +748,41 @@ def parse( groups = match.groupdict() title = groups["title"] admonition_type = groups["type"] - if admonition_type.lower() in _section_kind: + is_section = admonition_type.lower() in _section_kind + + has_previous_line = offset > 0 + blank_line_above = not has_previous_line or _is_empty_line(lines[offset - 1]) + has_next_line = offset < len(lines) - 1 + has_next_lines = offset < len(lines) - 2 + blank_line_below = has_next_line and _is_empty_line(lines[offset + 1]) + blank_lines_below = has_next_lines and _is_empty_line(lines[offset + 2]) + indented_line_below = has_next_line and not blank_line_below and lines[offset + 1].startswith(" ") + indented_lines_below = has_next_lines and not blank_lines_below and lines[offset + 2].startswith(" ") + if not (indented_line_below or indented_lines_below): + # Do not warn when there are no contents, + # this is most probably not a section or admonition. + current_section.append(lines[offset]) + offset += 1 + continue + reasons = [] + kind = "section" if is_section else "admonition" + if (indented_line_below or indented_lines_below) and not blank_line_above: + reasons.append(f"Missing blank line above {kind}") + if indented_lines_below and blank_line_below: + reasons.append(f"Extraneous blank line below {kind} title") + if reasons: + reasons_string = "; ".join(reasons) + _warn( + docstring, + offset, + f"Possible {kind} skipped, reasons: {reasons_string}", + LogLevel.debug, + ) + current_section.append(lines[offset]) + offset += 1 + continue + + if is_section: if current_section: if any(current_section): sections.append(DocstringSectionText("\n".join(current_section).rstrip("\n"))) diff --git a/src/griffe/docstrings/utils.py b/src/griffe/docstrings/utils.py index b17ed220..ac3e9463 100644 --- a/src/griffe/docstrings/utils.py +++ b/src/griffe/docstrings/utils.py @@ -4,7 +4,7 @@ from ast import PyCF_ONLY_AST from contextlib import suppress -from typing import TYPE_CHECKING, Callable +from typing import TYPE_CHECKING, Protocol from griffe.agents.nodes import safe_get_annotation from griffe.logger import LogLevel, get_logger @@ -14,7 +14,12 @@ from griffe.expressions import Expr -def warning(name: str) -> Callable[[Docstring, int, str], None]: +class WarningCallable(Protocol): + def __call__(self, docstring: Docstring, offset: int, message: str, log_level: LogLevel = ...) -> None: + ... + + +def warning(name: str) -> WarningCallable: """Create and return a warn function. Parameters: @@ -32,12 +37,13 @@ def warning(name: str) -> Callable[[Docstring, int, str], None]: """ logger = get_logger(name) - def warn(docstring: Docstring, offset: int, message: str) -> None: + def warn(docstring: Docstring, offset: int, message: str, log_level: LogLevel = LogLevel.warning) -> None: try: prefix = docstring.parent.relative_filepath # type: ignore[union-attr] except (AttributeError, ValueError): prefix = "" - logger.warning(f"{prefix}:{(docstring.lineno or 0)+offset}: {message}") + log = getattr(logger, log_level.value) + log(f"{prefix}:{(docstring.lineno or 0)+offset}: {message}") return warn diff --git a/tests/conftest.py b/tests/conftest.py index 137d04b7..3be27baf 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1 @@ """Configuration for the pytest test suite.""" - -from __future__ import annotations - -pytest_plugins = ["griffe.tests"] diff --git a/tests/test_docstrings/helpers.py b/tests/test_docstrings/helpers.py index 647dd2f2..e29309d5 100644 --- a/tests/test_docstrings/helpers.py +++ b/tests/test_docstrings/helpers.py @@ -6,6 +6,7 @@ from griffe.dataclasses import Attribute, Class, Docstring, Function, Module from griffe.docstrings.dataclasses import DocstringAttribute, DocstringElement, DocstringParameter, DocstringSection +from griffe.logger import LogLevel if TYPE_CHECKING: from types import ModuleType @@ -53,7 +54,7 @@ def parse(docstring: str, parent: ParentType | None = None, **parser_opts: Any) docstring_object.parent = parent parent.docstring = docstring_object warnings = [] - parser_module._warn = lambda _docstring, _offset, message: warnings.append(message) # type: ignore[attr-defined] + parser_module._warn = lambda _docstring, _offset, message, log_level=LogLevel.warning: warnings.append(message) # type: ignore[attr-defined] sections = parser_module.parse(docstring_object, **parser_opts) return sections, warnings diff --git a/tests/test_docstrings/test_google.py b/tests/test_docstrings/test_google.py index 7d74b0d8..82b5e4ba 100644 --- a/tests/test_docstrings/test_google.py +++ b/tests/test_docstrings/test_google.py @@ -515,7 +515,7 @@ def test_parse_yields_section(parse_google: ParserType) -> None: def test_invalid_sections(parse_google: ParserType) -> None: - """Warn on invalid (empty) sections. + """Warn on invalid sections. Parameters: parse_google: Fixture parser. @@ -533,39 +533,7 @@ def test_invalid_sections(parse_google: ParserType) -> None: sections, warnings = parse_google(docstring) assert len(sections) == 1 - for warning in warnings[:3]: - assert "Empty" in warning - assert "Empty returns section at line" in warnings[3] - assert "Empty" in warnings[-1] - - -def test_close_sections(parse_google: ParserType) -> None: - """Parse sections without blank lines in between. - - Parameters: - parse_google: Fixture parser. - """ - docstring = """ - Parameters: - x: X. - Parameters: - y: Y. - - Parameters: - z: Z. - Exceptions: - Error2: error. - Exceptions: - Error1: error. - Returns: - 1. - Returns: - 2. - """ - - sections, warnings = parse_google(docstring) - assert len(sections) == 7 - assert len(warnings) == 5 # no type or annotations + assert not warnings # ============================================================================================= @@ -797,10 +765,9 @@ def test_parameter_line_without_colon(parse_google: ParserType) -> None: """ sections, warnings = parse_google(docstring) - assert not sections # getting x fails, so the section is empty and discarded - assert len(warnings) == 2 + assert len(sections) == 0 # empty sections are discarded + assert len(warnings) == 1 assert "pair" in warnings[0] - assert "Empty" in warnings[1] def test_parameter_line_without_colon_keyword_only(parse_google: ParserType) -> None: @@ -815,10 +782,9 @@ def test_parameter_line_without_colon_keyword_only(parse_google: ParserType) -> """ sections, warnings = parse_google(docstring) - assert not sections # getting x fails, so the section is empty and discarded - assert len(warnings) == 2 + assert len(sections) == 0 # empty sections are discarded + assert len(warnings) == 1 assert "pair" in warnings[0] - assert "Empty" in warnings[1] def test_warn_about_unknown_parameters(parse_google: ParserType) -> None: @@ -1285,7 +1251,6 @@ def test_ignore_init_summary(parse_google: ParserType, docstring: str) -> None: """, r""" Examples: - Base case 2. We have a blankline test. >>> print("a\n\nb") a @@ -1320,7 +1285,6 @@ def test_trim_doctest_flags_multi_example(parse_google: ParserType) -> None: """ docstring = r""" Examples: - Test multiline example blocks. We want to skip the following test. >>> 1 + 1 == 3 # doctest: +SKIP @@ -1430,3 +1394,49 @@ def test_parse_returns_multiple_items( assert annotated.name == expected_.name assert str(annotated.annotation) == str(expected_.annotation) assert annotated.description == expected_.description + + +def test_avoid_false_positive_sections(parse_google: ParserType) -> None: + """Avoid false positive when parsing sections. + + Parameters: + parse_google: Fixture parser. + """ + docstring = """ + Summary. + Modules: + Not a modules section. + No blank line before title: + Not an admonition. + + Blank line after title: + + Not an admonition. + + Modules: + + Not a modules section. + Modules: + + Not a modules section. + No blank line before and blank line after: + + Not an admonition. + + Classes: + + - Text. + """ + sections, warnings = parse_google(docstring) + assert len(sections) == 1 + assert "Classes" in sections[0].value + assert "Text" in sections[0].value + assert len(warnings) == 6 + assert warnings == [ + "Possible section skipped, reasons: Missing blank line above section", + "Possible admonition skipped, reasons: Missing blank line above admonition", + "Possible admonition skipped, reasons: Extraneous blank line below admonition title", + "Possible section skipped, reasons: Extraneous blank line below section title", + "Possible section skipped, reasons: Missing blank line above section; Extraneous blank line below section title", + "Possible admonition skipped, reasons: Missing blank line above admonition; Extraneous blank line below admonition title", + ]