From b0bc6cd63dbde86df202c07691cceea46caf3a5b Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sat, 31 Oct 2026 09:01:11 -0600 Subject: [PATCH 01/10] scripts: add a docstring checker that compares a branch against main Collects every public API documentation gap at the working tree and at a base git ref with griffe, and reports only the gaps the branch introduces. Three rules: a public module, class, function or method with no docstring, a parameter with no entry in an Args: section, and a function whose return annotation is not None with no Returns: or Yields: section. Comparing rather than measuring against a threshold lets the repository adopt the check while gaps remain, and still refuses a pull request that adds a new one. --- scripts/docstring_checker.py | 336 +++++++++++++++++++++++++++++++++++ 1 file changed, 336 insertions(+) create mode 100644 scripts/docstring_checker.py diff --git a/scripts/docstring_checker.py b/scripts/docstring_checker.py new file mode 100644 index 00000000000..cad4a1206e0 --- /dev/null +++ b/scripts/docstring_checker.py @@ -0,0 +1,336 @@ +# Copyright The OpenTelemetry Authors +# SPDX-License-Identifier: Apache-2.0 + +"""Fail when a branch leaves a public API object less documented than main does. + +The check is a comparison rather than an absolute threshold. It collects every +documentation gap in the public API at the current working tree, collects the +same set at a base git ref, and reports only the gaps the branch introduces. +That lets the repository adopt the check while gaps remain, and still refuses +any pull request that adds a new one. + +Three kinds of gap are reported, named by the ``Gap.rule`` strings +``missing-docstring``, ``undocumented-parameter`` and ``missing-returns``. +""" + +from __future__ import annotations + +from argparse import ArgumentParser, ArgumentDefaultsHelpFormatter +from dataclasses import dataclass +from logging import ERROR, getLogger +from sys import exit as sys_exit, stderr + +from griffe import ( + Alias, + Class, + DocstringSectionKind, + Function, + Kind, + Module, + Object, + Parser, + load, + load_git, +) + +from eachdist import find_projectroot, find_targets + +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 + + +def main() -> int: + """Run the check and report. + + Returns: + ``0`` when the branch introduces no gap, ``1`` when it introduces at + least one. + """ + parser = ArgumentParser( + description="Fail when a branch introduces a public API documentation gap", + formatter_class=ArgumentDefaultsHelpFormatter, + ) + parser.add_argument( + "--module", + default=ROOT_MODULE, + help="Name of the module to check", + ) + parser.add_argument( + "--against", + default="main", + help="Git ref to compare against", + ) + arguments = parser.parse_args() + + getLogger("griffe").setLevel(ERROR) + + paths: list[str] = search_paths() + current: set[Gap] = collect_gaps( + load(arguments.module, search_paths=paths, docstring_parser=Parser.google) + ) + base: set[Gap] = collect_gaps( + load_git( + arguments.module, + ref=arguments.against, + search_paths=paths, + docstring_parser=Parser.google, + ) + ) + + introduced: list[Gap] = sorted(current - base) + closed: list[Gap] = sorted(base - current) + + if closed: + print(f"This branch closes {len(closed)} documentation gaps. Thank you.") + print() + + if not introduced: + print("This branch introduces no public API documentation gaps.") + return 0 + + print("This branch introduces the following public API documentation gaps:", file=stderr) + print(file=stderr) + for gap in introduced: + print(f" {gap}", file=stderr) + print(file=stderr) + print( + "Every public module, class, function and method needs a docstring, " + "every parameter needs an entry in an Args: section, and every " + "function whose annotation returns a value needs a Returns: section. " + "Constructor parameters belong on the class docstring, because " + "docs/conf.py does not set special-members and an __init__ docstring " + "is therefore never rendered.", + file=stderr, + ) + return 1 + + +if __name__ == "__main__": + sys_exit(main()) From 432452bfdf60d605b08b6204ecba37b39bf3ff2b Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sat, 31 Oct 2026 09:01:11 -0600 Subject: [PATCH 02/10] scripts: test the docstring checker Covers each of the three rules, the cases that must not be reported (private members, dunder methods other than __init__, an unannotated return, self and cls), the convention that constructor parameters are documented on the class docstring, and the skip list for generated trees. --- scripts/tests/test-requirements.txt | 1 + scripts/tests/test_docstring_checker.py | 330 ++++++++++++++++++++++++ 2 files changed, 331 insertions(+) create mode 100644 scripts/tests/test_docstring_checker.py 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..8086c953e76 --- /dev/null +++ b/scripts/tests/test_docstring_checker.py @@ -0,0 +1,330 @@ +# 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)" + ) From 345d81235f1b727d7bd04c4c535c657c8a35a0c1 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sat, 31 Oct 2026 09:01:11 -0600 Subject: [PATCH 03/10] tox: add the docstring-check environment --- tox.ini | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tox.ini b/tox.ini index 0c23ae68234..fbe0e19b738 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,16 @@ commands = python {toxinidir}/scripts/griffe_check.py python {toxinidir}/scripts/public_symbols_checker.py +[testenv:docstring-check] +recreate = True +deps = + -c dev-requirements.txt + GitPython + griffe + toml +commands = + python {toxinidir}/scripts/docstring_checker.py + [testenv:generate-workflows] deps = tox From 00178c1b4640e92ecb0bc34e5340077ad58786a3 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sat, 31 Oct 2026 09:01:11 -0600 Subject: [PATCH 04/10] workflows: run docstring-check on pull requests The job needs the same git setup as public-symbols-check, because the checker loads the base ref: full history and a local main. It carries its own label, Approve Docstring check, so that approving a public API change does not also waive the docstring gate. --- .github/workflows/misc.yml | 33 +++++++++++++++++++++++++ .github/workflows/templates/misc.yml.j2 | 9 +++++-- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/.github/workflows/misc.yml b/.github/workflows/misc.yml index 237cd3f30bc..eac8ac23d15 100644 --- a/.github/workflows/misc.yml +++ b/.github/workflows/misc.yml @@ -279,6 +279,39 @@ 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') + && github.actor != 'otelbot[bot]' && github.event_name == 'pull_request' + steps: + - name: Checkout repo @ SHA - ${{ github.sha }} + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + fetch-depth: 0 + - name: Checkout main + run: git checkout main + + - name: Pull origin + run: git pull --rebase=false origin main + + - name: Checkout pull request + run: git checkout ${{ github.event.pull_request.head.sha }} + + - 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..f2695fc0c29 100644 --- a/.github/workflows/templates/misc.yml.j2 +++ b/.github/workflows/templates/misc.yml.j2 @@ -45,6 +45,11 @@ 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') + && github.actor != 'otelbot[bot]' && github.event_name == 'pull_request' + {%- endif %} {%- if job_data == "docs" %} if: | github.event_name == 'merge_group' || @@ -55,7 +60,7 @@ jobs: - name: Checkout repo @ SHA - ${% raw %}{{ github.sha }}{% endraw %} uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - persist-credentials: false{% if job_data == "public-symbols-check" %} + persist-credentials: false{% if job_data in ("public-symbols-check", "docstring-check") %} fetch-depth: 0{% endif %} {%- if job_data in contrib_job_datas %} @@ -67,7 +72,7 @@ jobs: ref: ${% raw %}{{ env.CONTRIB_REPO_SHA }}{% endraw %} path: opentelemetry-python-contrib {%- endif %} - {%- if job_data == "public-symbols-check" %} + {%- if job_data in ("public-symbols-check", "docstring-check") %} - name: Checkout main run: git checkout main From 6a0198bddcc543346c59dabd323e39590afe355e Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sat, 31 Oct 2026 09:01:11 -0600 Subject: [PATCH 05/10] docs: describe the docstring requirements and the check that enforces them --- CONTRIBUTING.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6a4645af6c3..d6da4b9e8dd 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,20 @@ 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. + +To check whether your PR leaves a public symbol less documented than `main` does, run +`tox -e docstring-check`. It compares the documentation gaps at your branch against the gaps at +`main` and fails only on the ones your branch introduces, so the repository can close the gaps it +already has at its own pace while no PR adds a new one. 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 From 5e53a12c6ec0767f6c0dc18e373ba5f18d1d54b7 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sat, 31 Oct 2026 09:02:37 -0600 Subject: [PATCH 06/10] changelog: add the fragment for the docstring check --- .changelog/79.added | 1 + 1 file changed, 1 insertion(+) create mode 100644 .changelog/79.added 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 From f6cf150e02b07f23757fccbf62993156793edbab Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sat, 31 Oct 2026 09:16:10 -0600 Subject: [PATCH 07/10] scripts: check every public API object, not only the ones a branch adds The check now reports every documentation gap in the public API rather than the set difference against a base ref, so existing undocumented code fails it too. There are 1369 gaps at 3462000f3: 987 missing-docstring, 242 undocumented-parameter, 140 missing-returns. --against still subtracts the gaps of a ref, which narrows a long report to the gaps one working tree adds. It is a reading aid rather than the gate. --- scripts/docstring_checker.py | 118 ++++++++++++++---------- scripts/tests/test_docstring_checker.py | 4 +- 2 files changed, 70 insertions(+), 52 deletions(-) diff --git a/scripts/docstring_checker.py b/scripts/docstring_checker.py index cad4a1206e0..0e8929625ed 100644 --- a/scripts/docstring_checker.py +++ b/scripts/docstring_checker.py @@ -1,25 +1,33 @@ # Copyright The OpenTelemetry Authors # SPDX-License-Identifier: Apache-2.0 -"""Fail when a branch leaves a public API object less documented than main does. +"""Fail when a public API object is not properly documented. -The check is a comparison rather than an absolute threshold. It collects every -documentation gap in the public API at the current working tree, collects the -same set at a base git ref, and reports only the gaps the branch introduces. -That lets the repository adopt the check while gaps remain, and still refuses -any pull request that adds a new one. +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 ArgumentParser, ArgumentDefaultsHelpFormatter +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, stderr +from sys import exit as sys_exit +from sys import stderr +from eachdist import find_projectroot, find_targets from griffe import ( Alias, Class, @@ -33,8 +41,6 @@ load_git, ) -from eachdist import find_projectroot, find_targets - DIRECTORIES_NOT_SEARCHED: tuple[str, ...] = ( "docs", "scripts", @@ -114,8 +120,7 @@ def has_returns_section(obj: Object) -> bool: if obj.docstring is None: return False return any( - section.kind in (DocstringSectionKind.returns, DocstringSectionKind.yields) - for section in obj.docstring.parsed + section.kind in (DocstringSectionKind.returns, DocstringSectionKind.yields) for section in obj.docstring.parsed ) @@ -266,15 +271,30 @@ def search_paths() -> list[str]: 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 the branch introduces no gap, ``1`` when it introduces at - least one. + ``0`` when no gap is reported, ``1`` when at least one is. """ parser = ArgumentParser( - description="Fail when a branch introduces a public API documentation gap", + description="Fail when a public API object is not properly documented", formatter_class=ArgumentDefaultsHelpFormatter, ) parser.add_argument( @@ -284,51 +304,51 @@ def main() -> int: ) parser.add_argument( "--against", - default="main", - help="Git ref to compare 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() - current: set[Gap] = collect_gaps( - load(arguments.module, search_paths=paths, docstring_parser=Parser.google) - ) - base: set[Gap] = collect_gaps( - load_git( - arguments.module, - ref=arguments.against, - search_paths=paths, - docstring_parser=Parser.google, + 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, + ) ) - ) - - introduced: list[Gap] = sorted(current - base) - closed: list[Gap] = sorted(base - current) - - if closed: - print(f"This branch closes {len(closed)} documentation gaps. Thank you.") - print() - - if not introduced: - print("This branch introduces no public API documentation gaps.") + 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 - print("This branch introduces the following public API documentation gaps:", file=stderr) + counts: Counter = Counter(gap.rule for gap in found) + for gap in sorted(found): + print(gap, file=stderr) print(file=stderr) - for gap in introduced: - print(f" {gap}", 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) - print( - "Every public module, class, function and method needs a docstring, " - "every parameter needs an entry in an Args: section, and every " - "function whose annotation returns a value needs a Returns: section. " - "Constructor parameters belong on the class docstring, because " - "docs/conf.py does not set special-members and an __init__ docstring " - "is therefore never rendered.", - file=stderr, - ) + + for rule in sorted(counts): + print(f"{rule}: {RULE_EXPLANATIONS[rule]}", file=stderr) return 1 diff --git a/scripts/tests/test_docstring_checker.py b/scripts/tests/test_docstring_checker.py index 8086c953e76..d44db8324ce 100644 --- a/scripts/tests/test_docstring_checker.py +++ b/scripts/tests/test_docstring_checker.py @@ -325,6 +325,4 @@ def test_generated_trees_are_not_walked(tmp_path, monkeypatch): 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)" - ) + assert str(checker.Gap("a.b", "undocumented-parameter", "x")) == "a.b: undocumented-parameter (x)" From d9e851c192886bf18c6655a7fea89860289841e4 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sat, 31 Oct 2026 09:16:10 -0600 Subject: [PATCH 08/10] tox: pass posargs through to the docstring checker So that tox -e docstring-check -- --against main works. --- tox.ini | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tox.ini b/tox.ini index fbe0e19b738..a76b781427e 100644 --- a/tox.ini +++ b/tox.ini @@ -486,14 +486,13 @@ commands = python {toxinidir}/scripts/public_symbols_checker.py [testenv:docstring-check] -recreate = True deps = -c dev-requirements.txt GitPython griffe toml commands = - python {toxinidir}/scripts/docstring_checker.py + python {toxinidir}/scripts/docstring_checker.py {posargs} [testenv:generate-workflows] deps = From 5173ca4a1db9aff21ef395a2cdef763391087925 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sat, 31 Oct 2026 09:16:10 -0600 Subject: [PATCH 09/10] workflows: simplify the docstring-check job The absolute check reads no base ref, so the job needs neither the full history nor a local main. Dropping the pull_request condition lets it run on merge groups and pushes as well, which an absolute check should. --- .github/workflows/misc.yml | 10 ---------- .github/workflows/templates/misc.yml.j2 | 5 ++--- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/.github/workflows/misc.yml b/.github/workflows/misc.yml index eac8ac23d15..d6d6b9e8fa4 100644 --- a/.github/workflows/misc.yml +++ b/.github/workflows/misc.yml @@ -285,21 +285,11 @@ jobs: timeout-minutes: 30 if: | !contains(github.event.pull_request.labels.*.name, 'Approve Docstring check') - && github.actor != 'otelbot[bot]' && github.event_name == 'pull_request' steps: - name: Checkout repo @ SHA - ${{ github.sha }} uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false - fetch-depth: 0 - - name: Checkout main - run: git checkout main - - - name: Pull origin - run: git pull --rebase=false origin main - - - name: Checkout pull request - run: git checkout ${{ github.event.pull_request.head.sha }} - name: Set up Python 3.11 uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 diff --git a/.github/workflows/templates/misc.yml.j2 b/.github/workflows/templates/misc.yml.j2 index f2695fc0c29..1114728e5f3 100644 --- a/.github/workflows/templates/misc.yml.j2 +++ b/.github/workflows/templates/misc.yml.j2 @@ -48,7 +48,6 @@ jobs: {%- if job_data == "docstring-check" %} if: | !contains(github.event.pull_request.labels.*.name, 'Approve Docstring check') - && github.actor != 'otelbot[bot]' && github.event_name == 'pull_request' {%- endif %} {%- if job_data == "docs" %} if: | @@ -60,7 +59,7 @@ jobs: - name: Checkout repo @ SHA - ${% raw %}{{ github.sha }}{% endraw %} uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - persist-credentials: false{% if job_data in ("public-symbols-check", "docstring-check") %} + persist-credentials: false{% if job_data == "public-symbols-check" %} fetch-depth: 0{% endif %} {%- if job_data in contrib_job_datas %} @@ -72,7 +71,7 @@ jobs: ref: ${% raw %}{{ env.CONTRIB_REPO_SHA }}{% endraw %} path: opentelemetry-python-contrib {%- endif %} - {%- if job_data in ("public-symbols-check", "docstring-check") %} + {%- if job_data == "public-symbols-check" %} - name: Checkout main run: git checkout main From 1e18558265f6de9a53587402fe438b2459dcad55 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sat, 31 Oct 2026 09:16:10 -0600 Subject: [PATCH 10/10] docs: describe the docstring check as absolute --- CONTRIBUTING.md | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d6da4b9e8dd..e6c5f5fa753 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -157,11 +157,17 @@ needs a `Returns:` or `Yields:` section. Constructor parameters are documented o 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. -To check whether your PR leaves a public symbol less documented than `main` does, run -`tox -e docstring-check`. It compares the documentation gaps at your branch against the gaps at -`main` and fails only on the ones your branch introduces, so the repository can close the gaps it -already has at its own pace while no PR adds a new one. If a gap is deliberate, the PR will be -labeled with `Approve Docstring check` so that this check is not run. +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: