diff --git a/.changelog/79.added b/.changelog/79.added new file mode 100644 index 00000000000..c1981ad4850 --- /dev/null +++ b/.changelog/79.added @@ -0,0 +1 @@ +Add a `docstring-check` CI job that fails a pull request which adds an undocumented public API object diff --git a/.github/workflows/misc.yml b/.github/workflows/misc.yml index 237cd3f30bc..d6d6b9e8fa4 100644 --- a/.github/workflows/misc.yml +++ b/.github/workflows/misc.yml @@ -279,6 +279,29 @@ jobs: - name: Run tests run: uvx --with tox-uv tox -e public-symbols-check + docstring-check: + name: docstring-check + runs-on: ubuntu-latest + timeout-minutes: 30 + if: | + !contains(github.event.pull_request.labels.*.name, 'Approve Docstring check') + steps: + - name: Checkout repo @ SHA - ${{ github.sha }} + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Set up Python 3.11 + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.11" + + - name: Set up uv + uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 + + - name: Run tests + run: uvx --with tox-uv tox -e docstring-check + shellcheck: name: shellcheck runs-on: ubuntu-latest diff --git a/.github/workflows/templates/misc.yml.j2 b/.github/workflows/templates/misc.yml.j2 index 822f46220f1..1114728e5f3 100644 --- a/.github/workflows/templates/misc.yml.j2 +++ b/.github/workflows/templates/misc.yml.j2 @@ -45,6 +45,10 @@ jobs: !contains(github.event.pull_request.labels.*.name, 'Approve Public API check') && github.actor != 'otelbot[bot]' && github.event_name == 'pull_request' {%- endif %} + {%- if job_data == "docstring-check" %} + if: | + !contains(github.event.pull_request.labels.*.name, 'Approve Docstring check') + {%- endif %} {%- if job_data == "docs" %} if: | github.event_name == 'merge_group' || diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6a4645af6c3..e6c5f5fa753 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -69,6 +69,7 @@ You can run `tox` with the following arguments: - `tox -e ruff` to run ruff linter and formatter checks against the entire codebase - `tox -e typecheck` to run pyright against entire code base. - `tox -e public-symbols-check` to run public_symbols_checker.py. +- `tox -e docstring-check` to run docstring_checker.py. - `tox -e docker-tests-{otlpexporter,opencensus}` to run tests in both or either one location. - `tox -e tracecontext` to run integration tests for tracecontext. - `tox -e precommit` to run all `pre-commit` actions @@ -148,6 +149,26 @@ behind this is that every PR that adds/removes public symbols fails in CI, forci If after checking them, it is considered that they are indeed necessary, the PR will be labeled with `Approve Public API check` so that this check is not run. +### Docstrings + +Every public symbol needs a docstring, every parameter of a public function or method needs an entry +in an `Args:` section, and every public function or method whose return annotation is not `None` +needs a `Returns:` or `Yields:` section. Constructor parameters are documented on the class +docstring rather than on `__init__`, because `docs/conf.py` does not set `special-members`, so an +`__init__` docstring is never rendered in the API reference. + +Run `tox -e docstring-check` to list every public symbol that does not meet those requirements. +The check is absolute: it reports every gap in the public API, not only the gaps your PR adds, so a +PR that adds an undocumented symbol fails and so does a PR that changes nothing while an existing +symbol stays undocumented. + +Run `tox -e docstring-check -- --against main` to see only the gaps your working tree has and that +`main` does not. That narrows a long report down to your own share of it. It is a reading aid; the +check that gates CI is the absolute run. + +If a gap is deliberate, the PR will be labeled with `Approve Docstring check` so that this check is +not run. + Also, we try to keep our console output as clean as possible. Most of the time this means catching expected log messages in the test cases: ``` python diff --git a/scripts/docstring_checker.py b/scripts/docstring_checker.py new file mode 100644 index 00000000000..0e8929625ed --- /dev/null +++ b/scripts/docstring_checker.py @@ -0,0 +1,356 @@ +# Copyright The OpenTelemetry Authors +# SPDX-License-Identifier: Apache-2.0 + +"""Fail when a public API object is not properly documented. + +The check is absolute. It walks the whole public API at the current working +tree and reports every documentation gap it finds, whether the gap was added +by the branch under test or has been in the repository for years. A pull +request that adds an undocumented object therefore fails, and so does a pull +request that changes nothing while an existing object remains undocumented. + +Three kinds of gap are reported, named by the ``Gap.rule`` strings +``missing-docstring``, ``undocumented-parameter`` and ``missing-returns``. + +Passing ``--against `` narrows the report to the gaps the working tree +has and that ```` does not, which is how a contributor can see their own +share of the total while the total is still large. That mode is a reading aid +and not the gate: the gate is the absolute run. +""" + +from __future__ import annotations + +from argparse import ArgumentDefaultsHelpFormatter, ArgumentParser +from collections import Counter +from dataclasses import dataclass +from logging import ERROR, getLogger +from sys import exit as sys_exit +from sys import stderr + +from eachdist import find_projectroot, find_targets +from griffe import ( + Alias, + Class, + DocstringSectionKind, + Function, + Kind, + Module, + Object, + Parser, + load, + load_git, +) + +DIRECTORIES_NOT_SEARCHED: tuple[str, ...] = ( + "docs", + "scripts", + "opentelemetry-docker-tests", + "examples", + "_template", +) + +MODULES_NOT_CHECKED: tuple[str, ...] = ( + "opentelemetry.proto", + "opentelemetry.proto_json", + "opentelemetry.semconv", +) + +PARAMETERS_NEVER_DOCUMENTED: frozenset[str] = frozenset({"self", "cls"}) + +ROOT_MODULE: str = "opentelemetry" + + +@dataclass(frozen=True, order=True) +class Gap: + """One documentation gap at one public API object. + + Attributes: + path: The fully qualified name of the object, as griffe reports it. + This is the identity used to compare two refs, so a gap that + merely moves to a different line is not reported as new. + rule: Which of the three checks the object failed. + detail: What is missing, for example the name of an undocumented + parameter. The empty string when the rule needs no detail. + """ + + path: str + rule: str + detail: str = "" + + def __str__(self) -> str: + """Render the gap as one line of the report. + + Returns: + The qualified name, the rule and the detail, separated by spaces. + """ + suffix: str = f" ({self.detail})" if self.detail else "" + return f"{self.path}: {self.rule}{suffix}" + + +def documented_parameter_names(obj: Object) -> set[str]: + """Collect the parameter names a Google-style docstring documents. + + Args: + obj: Any griffe object. One with no docstring contributes nothing. + + Returns: + The names appearing in the ``Args:`` sections of the docstring. An + object with no docstring, or a docstring with no ``Args:`` section, + gives the empty set. + """ + if obj.docstring is None: + return set() + names: set[str] = set() + for section in obj.docstring.parsed: + if section.kind is DocstringSectionKind.parameters: + names.update(parameter.name for parameter in section.value) + return names + + +def has_returns_section(obj: Object) -> bool: + """Say whether a docstring documents a return value. + + Args: + obj: Any griffe object. One with no docstring documents nothing. + + Returns: + ``True`` when the docstring carries a ``Returns:`` or ``Yields:`` + section. + """ + if obj.docstring is None: + return False + return any( + section.kind in (DocstringSectionKind.returns, DocstringSectionKind.yields) for section in obj.docstring.parsed + ) + + +def returns_a_value(function: Function) -> bool: + """Say whether a function's annotation promises a value to its caller. + + An unannotated function is treated as returning nothing, so that adding + this check does not demand a ``Returns:`` section from code that has no + annotations at all. + + Args: + function: The function to inspect. + + Returns: + ``True`` when the return annotation is present and is not ``None``. + """ + annotation = function.returns + if annotation is None: + return False + return str(annotation) not in ("None", "NoneType") + + +def gaps_of_function(function: Function, parameters_documented_elsewhere: set[str]) -> list[Gap]: + """Collect the gaps of one public function or method. + + Args: + function: The function to inspect. + parameters_documented_elsewhere: Parameter names that another object's + docstring already documents. This carries the repository's + convention that constructor parameters are documented on the class + rather than on ``__init__``, which is what the rendered API + reference shows. + + Returns: + Every gap found, which is the empty list for a fully documented + function. + """ + if function.docstring is None: + return [Gap(function.path, "missing-docstring")] + + gaps: list[Gap] = [] + documented: set[str] = documented_parameter_names(function) | parameters_documented_elsewhere + for parameter in function.parameters: + if parameter.name in PARAMETERS_NEVER_DOCUMENTED: + continue + if parameter.name not in documented: + gaps.append(Gap(function.path, "undocumented-parameter", parameter.name)) + + if returns_a_value(function) and not has_returns_section(function): + gaps.append(Gap(function.path, "missing-returns")) + return gaps + + +def gaps_of_object(obj: Object) -> list[Gap]: + """Walk one public object and everything public inside it. + + Submodules are not descended into, because ``walk_modules`` already + enumerates every module and it is the only place that applies + ``MODULES_NOT_CHECKED``. Descending here as well would both report the + gaps of a generated tree and do the work of walking it twice. + + Args: + obj: The module or class to walk. + + Returns: + Every gap found in the object, its public functions, and its public + classes including the classes nested inside those. + """ + gaps: list[Gap] = [] + if obj.docstring is None: + gaps.append(Gap(obj.path, "missing-docstring")) + + constructor_parameters: set[str] = set() + if isinstance(obj, Class): + constructor_parameters = documented_parameter_names(obj) + + for member in obj.members.values(): + if isinstance(member, Alias): + continue + if not member.is_public: + continue + if member.kind is Kind.ATTRIBUTE: + continue + if isinstance(member, Function): + if member.name.startswith("__") and member.name != "__init__": + continue + documented_elsewhere = constructor_parameters if member.name == "__init__" else set() + if member.name == "__init__" and member.docstring is None and constructor_parameters: + gaps.extend(gaps_of_function(member, documented_elsewhere)[1:]) + continue + gaps.extend(gaps_of_function(member, documented_elsewhere)) + elif isinstance(member, Class): + gaps.extend(gaps_of_object(member)) + return gaps + + +def collect_gaps(root: Module) -> set[Gap]: + """Collect every documentation gap reachable from a loaded root module. + + Args: + root: The module griffe loaded, normally ``opentelemetry``. + + Returns: + The set of gaps, which is what two refs are compared on. + """ + gaps: set[Gap] = set() + for module in walk_modules(root): + gaps.update(gaps_of_object(module)) + return gaps + + +def walk_modules(root: Module) -> list[Module]: + """List the modules to check, skipping the generated trees. + + Args: + root: The module griffe loaded. + + Returns: + Every module at or below the root whose name is not under one of + ``MODULES_NOT_CHECKED``, in a deterministic order. + """ + found: list[Module] = [] + pending: list[Module] = [root] + while pending: + module = pending.pop() + if module.path.startswith(MODULES_NOT_CHECKED): + continue + found.append(module) + for member in module.members.values(): + if isinstance(member, Module) and not isinstance(member, Alias): + pending.append(member) + return sorted(found, key=lambda module: module.path) + + +def search_paths() -> list[str]: + """Build the list of source directories griffe searches. + + Returns: + One ``src`` directory per distribution in the repository, excluding + the directories in ``DIRECTORIES_NOT_SEARCHED``. + """ + rootpath = find_projectroot() + paths: list[str] = [] + for target in find_targets("DEFAULT", rootpath): + relative = target.relative_to(rootpath) + if not any(excluded in str(relative) for excluded in DIRECTORIES_NOT_SEARCHED): + paths.append(str(relative / "src")) + return paths + + +RULE_EXPLANATIONS: dict[str, str] = { + "missing-docstring": "This object has no docstring. Write one.", + "undocumented-parameter": ( + "This parameter has no entry in an Args: section. Add one. A " + "constructor parameter is documented on the class docstring rather " + "than on __init__, because docs/conf.py does not set special-members " + "and an __init__ docstring is therefore never rendered." + ), + "missing-returns": ( + "This function is annotated as returning a value and never says what " + "the value is. Add a Returns: section, or a Yields: section for a " + "generator." + ), +} + + +def main() -> int: + """Run the check and report. + + Returns: + ``0`` when no gap is reported, ``1`` when at least one is. + """ + parser = ArgumentParser( + description="Fail when a public API object is not properly documented", + formatter_class=ArgumentDefaultsHelpFormatter, + ) + parser.add_argument( + "--module", + default=ROOT_MODULE, + help="Name of the module to check", + ) + parser.add_argument( + "--against", + default=None, + help=( + "Git ref whose gaps are subtracted from the report, so that only " + "the gaps this working tree adds are shown. Omit it to report " + "every gap, which is what CI does" + ), + ) + arguments = parser.parse_args() + + getLogger("griffe").setLevel(ERROR) + + paths: list[str] = search_paths() + found: set[Gap] = collect_gaps(load(arguments.module, search_paths=paths, docstring_parser=Parser.google)) + + if arguments.against is not None: + base: set[Gap] = collect_gaps( + load_git( + arguments.module, + ref=arguments.against, + search_paths=paths, + docstring_parser=Parser.google, + ) + ) + closed: int = len(base - found) + found -= base + if closed: + print(f"This working tree closes {closed} documentation gaps that {arguments.against} has.") + print() + + if not found: + print("Every public API object checked is properly documented.") + return 0 + + counts: Counter = Counter(gap.rule for gap in found) + for gap in sorted(found): + print(gap, file=stderr) + print(file=stderr) + + for rule, count in sorted(counts.items()): + print(f"{count} {rule}", file=stderr) + print(f"{len(found)} documentation gaps in total.", file=stderr) + print(file=stderr) + + for rule in sorted(counts): + print(f"{rule}: {RULE_EXPLANATIONS[rule]}", file=stderr) + return 1 + + +if __name__ == "__main__": + sys_exit(main()) diff --git a/scripts/tests/test-requirements.txt b/scripts/tests/test-requirements.txt index 4adff2a01c7..9752b366fb7 100644 --- a/scripts/tests/test-requirements.txt +++ b/scripts/tests/test-requirements.txt @@ -1,2 +1,3 @@ +griffe==1.15.0 pytest==9.0.3 toml==0.10.2 diff --git a/scripts/tests/test_docstring_checker.py b/scripts/tests/test_docstring_checker.py new file mode 100644 index 00000000000..d44db8324ce --- /dev/null +++ b/scripts/tests/test_docstring_checker.py @@ -0,0 +1,328 @@ +# Copyright The OpenTelemetry Authors +# SPDX-License-Identifier: Apache-2.0 + +from importlib.util import module_from_spec, spec_from_file_location +from pathlib import Path +from sys import modules as sys_modules +from sys import path as sys_path +from textwrap import dedent + +from griffe import Parser, load + +SCRIPTS_DIRECTORY = Path(__file__).parents[1] +sys_path.insert(0, str(SCRIPTS_DIRECTORY)) + +_spec = spec_from_file_location("docstring_checker", SCRIPTS_DIRECTORY / "docstring_checker.py") +checker = module_from_spec(_spec) +sys_modules["docstring_checker"] = checker +_spec.loader.exec_module(checker) + + +def load_sample_module(tmp_path: Path, source: str, name: str = "mod"): + """Write one throwaway module and load it with griffe. + + Args: + tmp_path: The pytest temporary directory to write the package into. + source: The module source. It is dedented before being written. + name: The name of the module inside the package. + + Returns: + The griffe module, loaded with the Google docstring parser the + repository uses. + """ + package = tmp_path / "sample_pkg" + package.mkdir(exist_ok=True) + (package / "__init__.py").write_text('"""Package docstring."""\n') + (package / f"{name}.py").write_text(dedent(source)) + return load( + f"sample_pkg.{name}", + search_paths=[str(tmp_path)], + docstring_parser=Parser.google, + ) + + +def gaps_for(tmp_path: Path, source: str) -> set[tuple[str, str, str]]: + """Collect the gaps of one throwaway module built from source text. + + Args: + tmp_path: The pytest temporary directory to write the module into. + source: The module source. It is dedented before being written. + + Returns: + One tuple per gap, holding the qualified name, the rule and the + detail, so that assertions do not depend on the dataclass identity. + """ + module = load_sample_module(tmp_path, source) + return {(gap.path, gap.rule, gap.detail) for gap in checker.gaps_of_object(module)} + + +def test_fully_documented_module_has_no_gaps(tmp_path): + gaps = gaps_for( + tmp_path, + ''' + """Module docstring.""" + + + def visible(value: int) -> str: + """Summarize. + + Args: + value: The value. + + Returns: + The text. + """ + return str(value) + ''', + ) + assert gaps == set() + + +def test_missing_docstrings_are_reported(tmp_path): + gaps = gaps_for( + tmp_path, + """ + def visible(value): + return value + + + class Thing: + def method(self): + return None + """, + ) + assert ("sample_pkg.mod", "missing-docstring", "") in gaps + assert ("sample_pkg.mod.visible", "missing-docstring", "") in gaps + assert ("sample_pkg.mod.Thing", "missing-docstring", "") in gaps + assert ("sample_pkg.mod.Thing.method", "missing-docstring", "") in gaps + + +def test_undocumented_parameter_is_reported(tmp_path): + gaps = gaps_for( + tmp_path, + ''' + """Module docstring.""" + + + def visible(first: int, second: int) -> None: + """Summarize. + + Args: + first: The first value. + """ + ''', + ) + assert ("sample_pkg.mod.visible", "undocumented-parameter", "second") in gaps + assert ("sample_pkg.mod.visible", "undocumented-parameter", "first") not in gaps + + +def test_missing_returns_is_reported_only_when_a_value_is_returned(tmp_path): + gaps = gaps_for( + tmp_path, + ''' + """Module docstring.""" + + + def returns_value(value: int) -> str: + """Summarize. + + Args: + value: The value. + """ + return str(value) + + + def returns_nothing(value: int) -> None: + """Summarize. + + Args: + value: The value. + """ + ''', + ) + assert ("sample_pkg.mod.returns_value", "missing-returns", "") in gaps + assert ("sample_pkg.mod.returns_nothing", "missing-returns", "") not in gaps + + +def test_a_yields_section_satisfies_the_returns_rule(tmp_path): + gaps = gaps_for( + tmp_path, + ''' + """Module docstring.""" + + from typing import Iterator + + + def counted(value: int) -> Iterator[int]: + """Summarize. + + Args: + value: The value. + + Yields: + Each number below the value. + """ + yield from range(value) + ''', + ) + assert not any(rule == "missing-returns" for _, rule, _ in gaps) + + +def test_unannotated_return_is_not_required_to_be_documented(tmp_path): + gaps = gaps_for( + tmp_path, + ''' + """Module docstring.""" + + + def visible(value): + """Summarize. + + Args: + value: The value. + """ + return value + ''', + ) + assert not any(rule == "missing-returns" for _, rule, _ in gaps) + + +def test_constructor_parameters_may_be_documented_on_the_class(tmp_path): + gaps = gaps_for( + tmp_path, + ''' + """Module docstring.""" + + + class Thing: + """Hold a name. + + Args: + name: The name to hold. + """ + + def __init__(self, name: str): + """Initialize. Parameters are documented on the class.""" + self.name = name + ''', + ) + assert gaps == set() + + +def test_constructor_parameter_documented_nowhere_is_reported(tmp_path): + gaps = gaps_for( + tmp_path, + ''' + """Module docstring.""" + + + class Thing: + """Hold a name.""" + + def __init__(self, name: str): + """Initialize.""" + self.name = name + ''', + ) + assert ("sample_pkg.mod.Thing.__init__", "undocumented-parameter", "name") in gaps + + +def test_self_and_cls_are_never_required(tmp_path): + gaps = gaps_for( + tmp_path, + ''' + """Module docstring.""" + + + class Thing: + """Hold nothing.""" + + def method(self) -> None: + """Do nothing.""" + + @classmethod + def build(cls) -> None: + """Build nothing.""" + ''', + ) + assert gaps == set() + + +def test_private_members_are_not_checked(tmp_path): + gaps = gaps_for( + tmp_path, + ''' + """Module docstring.""" + + + def _private(value): + return value + + + class _Private: + def method(self): + return None + ''', + ) + assert gaps == set() + + +def test_dunder_methods_other_than_init_are_not_checked(tmp_path): + gaps = gaps_for( + tmp_path, + ''' + """Module docstring.""" + + + class Thing: + """Hold nothing.""" + + def __repr__(self) -> str: + return "Thing()" + ''', + ) + assert gaps == set() + + +def test_nested_classes_are_checked(tmp_path): + gaps = gaps_for( + tmp_path, + ''' + """Module docstring.""" + + + class Outer: + """Hold an inner class.""" + + class Inner: + def method(self) -> None: + """Do nothing.""" + ''', + ) + assert ("sample_pkg.mod.Outer.Inner", "missing-docstring", "") in gaps + + +def test_generated_trees_are_not_walked(tmp_path, monkeypatch): + package = tmp_path / "sample_pkg" + package.mkdir() + (package / "__init__.py").write_text('"""Package docstring."""\n') + for name in ("kept", "generated"): + subpackage = package / name + subpackage.mkdir() + (subpackage / "__init__.py").write_text("") + + root = load( + "sample_pkg", + search_paths=[str(tmp_path)], + docstring_parser=Parser.google, + ) + monkeypatch.setattr(checker, "MODULES_NOT_CHECKED", ("sample_pkg.generated",)) + walked = {module.path for module in checker.walk_modules(root)} + assert "sample_pkg.kept" in walked + assert "sample_pkg.generated" not in walked + assert {gap.path for gap in checker.collect_gaps(root)} == {"sample_pkg.kept"} + + +def test_gap_renders_with_and_without_detail(): + assert str(checker.Gap("a.b", "missing-docstring")) == "a.b: missing-docstring" + assert str(checker.Gap("a.b", "undocumented-parameter", "x")) == "a.b: undocumented-parameter (x)" diff --git a/tox.ini b/tox.ini index 0c23ae68234..a76b781427e 100644 --- a/tox.ini +++ b/tox.ini @@ -132,6 +132,7 @@ envlist = docker-tests-{otlpexporter,opencensus} docker-tests-declconfig public-symbols-check + docstring-check shellcheck generate-workflows precommit @@ -484,6 +485,15 @@ commands = python {toxinidir}/scripts/griffe_check.py python {toxinidir}/scripts/public_symbols_checker.py +[testenv:docstring-check] +deps = + -c dev-requirements.txt + GitPython + griffe + toml +commands = + python {toxinidir}/scripts/docstring_checker.py {posargs} + [testenv:generate-workflows] deps = tox