format_signatures: Fix whitespace issues and add tests (#381)

* format_signatures: Fix whitespace issues and add tests

* Update sphinx_immaterial/apidoc/format_signatures.py

Co-authored-by: Brendan <[email protected]>

* Exclude pformat snapshots since they aren't consistent across Sphinx/docutils versions

---------

Co-authored-by: Brendan <[email protected]>

noxfile.py

@@ -136,7 +136,7 @@ def check_eof(session: nox.Session):
 def check_trailing_space(session: nox.Session):
     """Ensure no trailing whitespace."""
     session.install("-r", "requirements/dev-pre_commit_hooks.txt")
-    all_files = get_file_list("*")
+    all_files = get_file_list("*", exclude=re.compile("tests/snapshots/.*"))
     # error output is super long and unhelpful; we only need the stdout in case of error
     ret = session.run(
         "trailing-whitespace-fixer", *all_files, silent=True, success_codes=[0, 1]

sphinx_immaterial/apidoc/format_signatures.py

@@ -151,6 +151,9 @@ def _transform_node(node):
         if isinstance(node, docutils.nodes.Text):
             return replacements.get(id(node)) or []
         new_node = node.copy()
+        # Ensure the new node does not contain any children. `desc_sig_space`,
+        # for example, adds a default child text node with content " ".
+        del new_node[:]
         new_node.extend(_transform_node_list(node.children, _transform_node))
         if len(new_node.children) == 0:
             # A pending_xref with no children is an error and indicates a
@@ -185,6 +188,30 @@ class FormatInputComponent(NamedTuple):
     """
 
 
+_INDENTED_LINE_PATTERN = re.compile(r".*(?:^|\n)([ \t]+)$", re.DOTALL)
+
+
+def _find_first_primary_entity_name_text_node(
+    adjusted_nodes: list[docutils.nodes.Node],
+) -> Optional[docutils.nodes.Text]:
+    for adjusted_node in adjusted_nodes:
+        node: docutils.nodes.Element
+        for node in adjusted_node.findall(
+            lambda node: (
+                isinstance(node, sphinx.addnodes.desc_addname)
+                or (
+                    isinstance(node, sphinx.addnodes.desc_name)
+                    and "sig-name-nonprimary" not in node["classes"]
+                )
+            )
+        ):
+            for text_node in cast(docutils.nodes.Element, node).findall(
+                docutils.nodes.Text
+            ):
+                return text_node
+    return None
+
+
 class FormatApplier:
     components: list[FormatInputComponent]
     adjusted_nodes: list[docutils.nodes.Node]
@@ -244,12 +271,71 @@ def apply(
         replacements: dict[int, list[docutils.nodes.Node]] = collections.defaultdict(
             list
         )
-        for tag, i1, i2, j1, j2 in difflib.SequenceMatcher(
-            " \t\n".__contains__,
-            formatted_input,
-            formatted_output,
-            autojunk=False,
-        ).get_opcodes():
+
+        def _get_opcodes():
+            return difflib.SequenceMatcher(
+                " \t\n".__contains__,
+                formatted_input,
+                formatted_output,
+                autojunk=False,
+            ).get_opcodes()
+
+        # Align `formatted_input` to `formatted_output`.
+        opcodes = _get_opcodes()
+
+        # Find the first text node of the entity name.
+        #
+        # Sometimes the formatter may indent the primary entity name, e.g.:
+        #
+        #     std::integral_constant<ptrdiff_t, N>
+        #         tensorstore::GetStaticOrDynamicExtent(span<X, N>);
+        #
+        # where the primary entity name is `tensorstore::GetStaticOrDynamicExtent`.
+        #
+        # This is not desirable for API documentation, and therefore is stripped
+        # out.
+        #
+        # To check for and remove such indentation, first locate the Text node
+        # corresponding to the start of the primary entity name.
+        first_name_text_node = _find_first_primary_entity_name_text_node(
+            self.adjusted_nodes
+        )
+
+        # Then determine the offests into `formatted_input` and
+        # `formatted_output` corresponding to `first_name_text_node`.
+        first_name_text_node_input_offset = 0
+        for component in components:
+            if component.node is first_name_text_node:
+                break
+            first_name_text_node_input_offset += len(component.orig_text)
+
+        # Determine output offset of `first_name_text_node_input_offset`.
+        first_name_text_node_output_offset = 0
+        for tag, i1, i2, j1, j2 in opcodes:
+            if i2 >= first_name_text_node_input_offset:
+                if tag == "equal":
+                    first_name_text_node_output_offset = j1 + (
+                        first_name_text_node_input_offset - i1
+                    )
+                else:
+                    first_name_text_node_output_offset = j1
+                break
+
+        # Check if `first_name_text_node` is indented on a new line.
+        if (
+            m := _INDENTED_LINE_PATTERN.fullmatch(
+                formatted_output, 0, first_name_text_node_output_offset
+            )
+        ) is not None:
+            # Strip leading whitespace, and recompute opcodes.
+            formatted_output = (
+                formatted_output[: m.start(1)]
+                + formatted_output[first_name_text_node_output_offset:]
+            )
+            opcodes = _get_opcodes()
+
+        # Compute the replacement text nodes for each component.
+        for tag, i1, i2, j1, j2 in opcodes:
             while True:
                 if i1 != i2 and i1 == component_end_offset:
                     component_start_offset = component_end_offset

tests/format_signatures_test.py

@@ -0,0 +1,63 @@
+import sphinx.addnodes
+
+
+TEST_SIGNATURES = {
+    "cpp_function": "cpp:function:: void foo(int something, int something_else, bool third_param, bool fourth_param, int fifth_param)",
+    "cpp_function_long": r"cpp:function:: template <typename T, \
+                                       typename U = void, \
+                                       int AnotherParameter = 42> \
+                             requires std::is_const_v<T> \
+                             const MyType LongFunctionSignatureExample(\
+                               const MyType bar, \
+                               uint8_t* arr, \
+                               unsigned int len = DEFAULT_LENGTH, \
+                               bool baz = false)",
+    "cpp_function_long_return_type": r"cpp:function:: std::integral_constant<ptrdiff_t, N> tensorstore::GetStaticOrDynamicExtent(span<X, N>);",
+    "py_function": r"py:function:: some_module.method_name( \
+                              some_parameter_with_a_long_name: \
+                                collections.abc.MutableMapping[\
+                                  tuple[str, float, numbers.Real], \
+                                  dict[int, tuple[list[frozenset[int]]]]], \
+                            ) -> collections.abc.MutableMapping[\
+                                   tuple[str, float, numbers.Real], \
+                                   dict[int, tuple[list[frozenset[int]]]]]",
+}
+
+
+def test_format_signatures(immaterial_make_app, snapshot):
+    app = immaterial_make_app(
+        extra_conf="""
+extensions.append("sphinx_immaterial.apidoc.format_signatures")
+object_description_options = [
+    ("cpp:.*", dict(clang_format_style={"BasedOnStyle": "LLVM"})),
+    ("py:.*", dict(black_format_style={})),
+]
+        """,
+        files={
+            "index.rst": "\n\n".join(
+                f"""
+.. {directive}
+
+   Synopsis goes here.
+"""
+                for directive in TEST_SIGNATURES.values()
+            )
+        },
+    )
+
+    app.build()
+
+    assert not app._warning.getvalue()
+
+    doc = app.env.get_and_resolve_doctree("index", app.builder)
+
+    formatted_signatures = {
+        identifier: signature
+        for identifier, signature in zip(
+            TEST_SIGNATURES.keys(),
+            doc.findall(condition=sphinx.addnodes.desc_signature),
+        )
+    }
+    for identifier in TEST_SIGNATURES.keys():
+        node = formatted_signatures[identifier]
+        snapshot.assert_match(node.astext(), f"{identifier}_astext.txt")

tests/requirements.txt

@@ -10,3 +10,5 @@ tests/issue_134/sphinx-immaterial-pybind11-issue-134
 -r ../requirements/json.txt
 -r ../requirements/jsonschema_validation.txt
 -r ../requirements/keys.txt
+-r ../requirements/black.txt
+-r ../requirements/clang-format.txt

tests/snapshots/format_signatures_test/test_format_signatures/cpp_function_astext.txt

@@ -0,0 +1,2 @@
+void foo(int something, int something_else, bool third_param,
+         bool fourth_param, int fifth_param);

tests/snapshots/format_signatures_test/test_format_signatures/cpp_function_long_astext.txt

@@ -0,0 +1,6 @@
+template <typename T, typename U = void, int AnotherParameter = 42>
+  requires std::is_const_v<T>
+const MyType
+LongFunctionSignatureExample(const MyType bar, uint8_t *arr,
+                             unsigned int len = DEFAULT_LENGTH,
+                             bool baz = false);

tests/snapshots/format_signatures_test/test_format_signatures/cpp_function_long_return_type_astext.txt

@@ -0,0 +1,2 @@
+std::integral_constant<ptrdiff_t, N>
+tensorstore::GetStaticOrDynamicExtent(span<X, N>);

tests/snapshots/format_signatures_test/test_format_signatures/py_function_astext.txt

@@ -0,0 +1,9 @@
+some_module.method_name(
+    some_parameter_with_a_long_name: collections.abc.MutableMapping[
+        tuple[str, float, numbers.Real],
+        dict[int, tuple[list[frozenset[int]]]],
+    ]
+) -> collections.abc.MutableMapping[
+    tuple[str, float, numbers.Real],
+    dict[int, tuple[list[frozenset[int]]]],
+]