👌 Improve need part processing (#1469)

The `need_part`/`np` role is not a reference and should not use an
`XRefRole` subclass (see #1437).

In addition, the processing now correctly handles and warns on need
parts that are not part of a need and `need` roles that reference an
unknown part.

Author: chrisjsewell

pyproject.toml

@@ -144,7 +144,7 @@ disable_error_code = ["no-redef"]
 
 legacy_tox_ini = """
 [tox]
-envlist = py10
+envlist = py310
 
 [testenv]
 usedevelop = true

sphinx_needs/logging.py

@@ -42,6 +42,7 @@ def get_logger(name: str) -> SphinxLoggerAdapter:
     "load_external_need",
     "load_service_need",
     "mpl",
+    "part",
     "title",
     "uml",
     "unknown_external_keys",
@@ -81,6 +82,7 @@ def get_logger(name: str) -> SphinxLoggerAdapter:
     "load_external_need": "Failed to load an external need",
     "load_service_need": "Failed to load a service need",
     "mpl": "Matplotlib required but not installed",
+    "part": "Error processing need part",
     "title": "Error creating need title",
     "uml": "Error in processing of UML diagram",
     "unknown_external_keys": "Unknown keys found in external need data",

sphinx_needs/needs.py

@@ -114,7 +114,7 @@
 from sphinx_needs.roles.need_func import NeedFunc, NeedFuncRole, process_need_func
 from sphinx_needs.roles.need_incoming import NeedIncoming, process_need_incoming
 from sphinx_needs.roles.need_outgoing import NeedOutgoing, process_need_outgoing
-from sphinx_needs.roles.need_part import NeedPart, process_need_part
+from sphinx_needs.roles.need_part import NeedPart, NeedPartRole, process_need_part
 from sphinx_needs.roles.need_ref import NeedRef, process_need_ref
 from sphinx_needs.services.github import GithubService
 from sphinx_needs.services.open_needs import OpenNeedsService
@@ -242,19 +242,8 @@ def setup(app: Sphinx) -> dict[str, Any]:
         ),
     )
 
-    app.add_role(
-        "need_part",
-        NeedsXRefRole(
-            nodeclass=NeedPart, innernodeclass=nodes.inline, warn_dangling=True
-        ),
-    )
-    # Shortcut for need_part
-    app.add_role(
-        "np",
-        NeedsXRefRole(
-            nodeclass=NeedPart, innernodeclass=nodes.inline, warn_dangling=True
-        ),
-    )
+    app.add_role("need_part", NeedPartRole())
+    app.add_role("np", NeedPartRole())  # Shortcut for need_part
 
     app.add_role(
         "need_count",

sphinx_needs/roles/need_part.py

@@ -3,31 +3,72 @@
 ---------------
 Provides the ability to mark specific parts of a need with an own id.
 
-Most voodoo is done in need.py
-
 """
 
 from __future__ import annotations
 
 import hashlib
 import re
 from collections.abc import Iterable
-from typing import cast
 
 from docutils import nodes
 from sphinx.application import Sphinx
 from sphinx.environment import BuildEnvironment
+from sphinx.util.docutils import SphinxRole
 from sphinx.util.nodes import make_refnode
 
 from sphinx_needs.data import NeedsInfoType, NeedsPartType
 from sphinx_needs.logging import get_logger, log_warning
 from sphinx_needs.nodes import Need
 
-log = get_logger(__name__)
+LOGGER = get_logger(__name__)
 
 
 class NeedPart(nodes.Inline, nodes.Element):
-    pass
+    @property
+    def title(self) -> str:
+        """Return the title of the part."""
+        return self.attributes["title"]  # type: ignore[no-any-return]
+
+    @property
+    def part_id(self) -> str:
+        """Return the ID of the part."""
+        return self.attributes["part_id"]  # type: ignore[no-any-return]
+
+    @property
+    def need_id(self) -> str | None:
+        """Return the ID of the need this part belongs to."""
+        return self.attributes["need_id"]  # type: ignore[no-any-return]
+
+    @need_id.setter
+    def need_id(self, value: str) -> None:
+        """Set the ID of the need this part belongs to."""
+        self.attributes["need_id"] = value
+
+
+_PART_PATTERN = re.compile(r"\(([\w-]+)\)(.*)", re.DOTALL)
+
+
+class NeedPartRole(SphinxRole):
+    """
+    Role for need parts, which are sub-needs of a need.
+    It is used to mark parts of a need with an own id.
+    """
+
+    def run(self) -> tuple[list[nodes.Node], list[nodes.system_message]]:
+        # note self.text is the content of the role, with backslash escapes removed
+        # TODO perhaps in a future change we should allow escaping parentheses in the part id?
+        # and also strip (unescaped) space before/after the title
+        result = _PART_PATTERN.match(self.text)
+        if result:
+            id_ = result.group(1)
+            title = result.group(2)
+        else:
+            id_ = hashlib.sha1(self.text.encode("UTF-8")).hexdigest().upper()[:3]
+            title = self.text
+        part = NeedPart(title=title, part_id=id_, need_id=None)
+        self.set_source_info(part)
+        return [part], []
 
 
 def process_need_part(
@@ -36,10 +77,16 @@ def process_need_part(
     fromdocname: str,
     found_nodes: list[nodes.Element],
 ) -> None:
-    pass
-
-
-part_pattern = re.compile(r"\(([\w-]+)\)(.*)", re.DOTALL)
+    # note this is called after needs have been processed and parts collected.
+    for node in found_nodes:
+        assert isinstance(node, NeedPart), "Expected NeedPart node"
+        if node.need_id is None:
+            log_warning(
+                LOGGER,
+                "Need part not associated with a need.",
+                "part",
+                node,
+            )
 
 
 def create_need_from_part(need: NeedsInfoType, part: NeedsPartType) -> NeedsInfoType:
@@ -67,58 +114,43 @@ def update_need_with_parts(
 ) -> None:
     app = env.app
     for part_node in part_nodes:
-        content = cast(str, part_node.children[0].children[0])  # ->inline->Text
-        result = part_pattern.match(content)
-        if result:
-            inline_id = result.group(1)
-            part_content = result.group(2)
-        else:
-            part_content = content
-            inline_id = (
-                hashlib.sha1(part_content.encode("UTF-8")).hexdigest().upper()[:3]
-            )
+        part_id = part_node.part_id
 
         if "parts" not in need:
             need["parts"] = {}
 
-        if inline_id in need["parts"]:
+        if part_id in need["parts"]:
             log_warning(
-                log,
+                LOGGER,
                 "part_need id {} in need {} is already taken. need_part may get overridden.".format(
-                    inline_id, need["id"]
+                    part_id, need["id"]
                 ),
                 "duplicate_part_id",
                 part_node,
             )
 
-        need["parts"][inline_id] = {
-            "id": inline_id,
-            "content": part_content,
+        need["parts"][part_id] = {
+            "id": part_id,
+            "content": part_node.title,
             "links": [],
             "links_back": [],
         }
 
-        part_id_ref = "{}.{}".format(need["id"], inline_id)
-
+        part_node.need_id = need["id"]
+        part_id_ref = "{}.{}".format(need["id"], part_id)
         part_node["reftarget"] = part_id_ref
 
-        part_text_node = nodes.Text(part_content)
-
-        part_node.children = []
         node_need_part_line = nodes.inline(ids=[part_id_ref], classes=["need-part"])
-        node_need_part_line.append(part_text_node)
+        node_need_part_line.append(nodes.Text(part_node.title))
 
         if docname := need["docname"]:
-            part_id_show = inline_id
-            part_link_text = f" {part_id_show}"
-            part_link_node = nodes.Text(part_link_text)
-
             part_ref_node = make_refnode(
-                app.builder, docname, docname, part_id_ref, part_link_node
+                app.builder, docname, docname, part_id_ref, nodes.Text(f" {part_id}")
             )
             part_ref_node["classes"] += ["needs-id"]
             node_need_part_line.append(part_ref_node)
 
+        part_node.children = []
         part_node.append(node_need_part_line)
 
 

sphinx_needs/roles/need_ref.py

@@ -85,7 +85,24 @@ def process_need_ref(
         need_id_full = node_need_ref["reftarget"]
         need_id_main, need_id_part = split_need_id(need_id_full)
 
-        if need_id_main in all_needs:
+        if need_id_main not in all_needs:
+            log_warning(
+                log,
+                f"linked need {node_need_ref['reftarget']} not found",
+                "link_ref",
+                location=node_need_ref,
+            )
+        elif (
+            need_id_part is not None
+            and need_id_part not in all_needs[need_id_main]["parts"]
+        ):
+            log_warning(
+                log,
+                f"linked need part {node_need_ref['reftarget']} not found",
+                "link_ref",
+                location=node_need_ref,
+            )
+        else:
             target_need = all_needs[need_id_main]
 
             dict_need = transform_need_to_dict(
@@ -162,12 +179,4 @@ def process_need_ref(
                     )
                     new_node_ref["classes"].append(target_need["external_css"])
 
-        else:
-            log_warning(
-                log,
-                f"linked need {node_need_ref['reftarget']} not found",
-                "link_ref",
-                location=node_need_ref,
-            )
-
         node_need_ref.replace_self(new_node_ref)

tests/doc_test/doc_need_parts/index.rst

@@ -23,6 +23,7 @@ NEED PARTS
 
         Part in nested need: :need_part:`(nested_id)something`
 
+:np:`hallo`
 
 :need:`SP_TOO_001.1`
 
@@ -31,3 +32,5 @@ NEED PARTS
 :need:`SP_TOO_001.awesome_id`
 
 :need:`My custom link name <SP_TOO_001.awesome_id>`
+
+:need:`SP_TOO_001.unknown_part`

tests/test_need_parts.py

@@ -19,7 +19,10 @@ def test_doc_need_parts(test_app, snapshot):
         app._warning.getvalue().replace(str(app.srcdir) + os.sep, "srcdir/")
     ).splitlines()
     # print(warnings)
-    assert warnings == []
+    assert warnings == [
+        "srcdir/index.rst:26: WARNING: Need part not associated with a need. [needs.part]",
+        "srcdir/index.rst:36: WARNING: linked need part SP_TOO_001.unknown_part not found [needs.link_ref]",
+    ]
 
     html = Path(app.outdir, "index.html").read_text()
     assert (