From 8aa192b357befc1273861e291996750ea902c9b9 Mon Sep 17 00:00:00 2001 From: Chris Sewell Date: Mon, 28 Oct 2024 19:37:36 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20FIX=20needflow=20graphviz=20link?= =?UTF-8?q?s=20(#1339)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Links are now initially calculated relative to the containing document. For PNGs, the links are defined as https://developer.mozilla.org/en-US/docs/Web/HTML/Element/map in the document, so this is correct. For SVGs, the graphs are extracted to external files, and in this case the links are modified by sphinx to be relative to the SVG file (from sphinx 7.2 onwards, see: https://github.com/sphinx-doc/sphinx/pull/11078) --- sphinx_needs/directives/needflow/_graphviz.py | 36 ++- tests/test_needflow.py | 255 ++++++++++-------- 2 files changed, 170 insertions(+), 121 deletions(-) diff --git a/sphinx_needs/directives/needflow/_graphviz.py b/sphinx_needs/directives/needflow/_graphviz.py index aa2572921..39b05805a 100644 --- a/sphinx_needs/directives/needflow/_graphviz.py +++ b/sphinx_needs/directives/needflow/_graphviz.py @@ -4,6 +4,7 @@ import textwrap from functools import lru_cache from typing import Callable, Literal, TypedDict +from urllib.parse import urlparse from docutils import nodes from sphinx.application import Sphinx @@ -17,9 +18,9 @@ from sphinx_needs.config import LinkOptionsType, NeedsSphinxConfig from sphinx_needs.data import NeedsInfoType, SphinxNeedsData from sphinx_needs.debug import measure_time -from sphinx_needs.diagrams_common import calculate_link from sphinx_needs.directives.needflow._directive import NeedflowGraphiz from sphinx_needs.directives.utils import no_needs_found_paragraph +from sphinx_needs.errors import NoUri from sphinx_needs.filter_common import ( filter_single_need, process_filters, @@ -159,7 +160,7 @@ def process_needflow_graphviz( node, needs_view, needs_config, - lambda n: calculate_link(app, n, fromdocname, relative="."), + lambda n: _get_link_to_need(app, fromdocname, n), id_comp_to_need, rendered_nodes, ) @@ -191,6 +192,31 @@ def process_needflow_graphviz( node.parent.parent.insert(node.parent.parent.index(node.parent) + 1, code) +def _get_link_to_need( + app: Sphinx, docname: str, need_info: NeedsInfoType +) -> str | None: + """Compute the link to a need, relative to a document. + + It is of note that the links are computed relative to the document that the graph is in. + For PNGs, the links are defined as https://developer.mozilla.org/en-US/docs/Web/HTML/Element/map in the document, so this correct. + For SVGs, the graphs are extracted to external files, and in this case the links are modified to be relative to the SVG file + (from sphinx 7.2 onwards, see: https://github.com/sphinx-doc/sphinx/pull/11078) + """ + if need_info["is_external"]: + if need_info["external_url"] and urlparse(need_info["external_url"]).scheme: + return need_info["external_url"] + elif need_info["docname"]: + try: + rel_uri = app.builder.get_relative_uri(docname, need_info["docname"]) + if not rel_uri: + # svg relative path fix cannot yet handle empty paths https://github.com/sphinx-doc/sphinx/issues/13078 + rel_uri = app.builder.get_target_uri(docname.split("/")[-1]) + except NoUri: + return None + return rel_uri + "#" + need_info["id_complete"] + return None + + class _RenderedNode(TypedDict): cluster_id: str | None need: NeedsInfoType @@ -206,7 +232,7 @@ def _render_node( node: NeedflowGraphiz, needs_view: NeedsView, config: NeedsSphinxConfig, - calc_link: Callable[[NeedsInfoType], str], + calc_link: Callable[[NeedsInfoType], str | None], id_comp_to_need: dict[str, NeedsInfoType], rendered_nodes: dict[str, _RenderedNode], subgraph: bool = True, @@ -285,7 +311,7 @@ def _render_subgraph( node: NeedflowGraphiz, needs_view: NeedsView, config: NeedsSphinxConfig, - calc_link: Callable[[NeedsInfoType], str], + calc_link: Callable[[NeedsInfoType], str | None], id_comp_to_need: dict[str, NeedsInfoType], rendered_nodes: dict[str, _RenderedNode], ) -> str: @@ -606,7 +632,7 @@ def html_visit_needflow_graphviz(self: HTML5Translator, node: NeedflowGraphiz) - log_warning(LOGGER, "Content has not been resolved", "needflow", location=node) raise nodes.SkipNode attrributes = node.attributes - format = self.builder.config.graphviz_output_format + format: Literal["png", "svg"] = self.builder.config.graphviz_output_format if format not in ("png", "svg"): log_warning( LOGGER, diff --git a/tests/test_needflow.py b/tests/test_needflow.py index a0aa492eb..66afa968c 100644 --- a/tests/test_needflow.py +++ b/tests/test_needflow.py @@ -3,6 +3,7 @@ import pytest from lxml import html as html_parser +from sphinx import version_info from sphinx.config import Config from sphinx.util.console import strip_colors @@ -36,6 +37,7 @@ def _get_svg(config: Config, outdir: Path, file: str, id: str) -> str: }, }, ], + ids=["plantuml", "graphviz"], indirect=True, ) def test_doc_build_html(test_app): @@ -50,32 +52,42 @@ def test_doc_build_html(test_app): assert warnings == "" outdir = Path(app.outdir) + svg = _get_svg(app.config, outdir, "index.html", "needflow-index-0") - for link in ( - "./index.html#SPEC_1", - "./index.html#SPEC_2", - "./index.html#STORY_1", - "./index.html#STORY_1.1", - "./index.html#STORY_1.2", - "./index.html#STORY_1.subspec", - "./index.html#STORY_2", - "./index.html#STORY_2.another_one", - ): - assert link in svg + + if test_app.config.needs_flow_engine == "graphviz" and version_info < (7, 2): + pass # links will be wrong due to https://github.com/sphinx-doc/sphinx/pull/11078 + else: + for link in ( + '"../index.html#SPEC_1"', + '"../index.html#SPEC_2"', + '"../index.html#STORY_1"', + '"../index.html#STORY_1.1"', + '"../index.html#STORY_1.2"', + '"../index.html#STORY_1.subspec"', + '"../index.html#STORY_2"', + '"../index.html#STORY_2.another_one"', + ): + assert link in svg + assert "No needs passed the filters" in Path(app.outdir, "index.html").read_text() svg = _get_svg(app.config, outdir, "page.html", "needflow-page-0") - for link in ( - "./index.html#SPEC_1", - "./index.html#SPEC_2", - "./index.html#STORY_1", - "./index.html#STORY_1.1", - "./index.html#STORY_1.2", - "./index.html#STORY_1.subspec", - "./index.html#STORY_2", - "./index.html#STORY_2.another_one", - ): - assert link in svg + + if test_app.config.needs_flow_engine == "graphviz" and version_info < (7, 2): + pass # links will be wrong due to https://github.com/sphinx-doc/sphinx/pull/11078 + else: + for link in ( + '"../index.html#SPEC_1"', + '"../index.html#SPEC_2"', + '"../index.html#STORY_1"', + '"../index.html#STORY_1.1"', + '"../index.html#STORY_1.2"', + '"../index.html#STORY_1.subspec"', + '"../index.html#STORY_2"', + '"../index.html#STORY_2.another_one"', + ): + assert link in svg svg = _get_svg( app.config, @@ -83,7 +95,7 @@ def test_doc_build_html(test_app): "needflow_with_root_id.html", "needflow-needflow_with_root_id-0", ) - print(svg) + for link in ("SPEC_1", "STORY_1", "STORY_2"): assert link in svg @@ -112,6 +124,7 @@ def test_doc_build_html(test_app): }, }, ], + ids=["plantuml", "graphviz"], indirect=True, ) def test_doc_build_needflow_incl_child_needs(test_app): @@ -128,97 +141,107 @@ def test_doc_build_needflow_incl_child_needs(test_app): outdir = Path(app.outdir) svg = _get_svg(app.config, outdir, "index.html", "needflow-index-0") - for link in ( - "./index.html#STORY_1", - "./index.html#STORY_1.1", - "./index.html#STORY_1.2", - "./index.html#STORY_2", - "./index.html#STORY_2.3", - "./index.html#SPEC_1", - "./index.html#SPEC_2", - "./index.html#SPEC_3", - "./index.html#SPEC_4", - "./index.html#STORY_3", - "./index.html#SPEC_5", - ): - assert link in svg - - svg = _get_svg( - app.config, - outdir, - "single_parent_need_filer.html", - "needflow-single_parent_need_filer-0", - ) - assert "./index.html#STORY_3" in svg - for link in ( - "./index.html#STORY_1", - "./index.html#STORY_1.1", - "./index.html#STORY_1.2", - "./index.html#STORY_2", - "./index.html#STORY_2.3", - "./index.html#SPEC_1", - "./index.html#SPEC_2", - "./index.html#SPEC_3", - "./index.html#SPEC_4", - "./index.html#SPEC_5", - ): - assert link not in svg - svg = _get_svg( - app.config, - outdir, - "single_child_with_child_need_filter.html", - "needflow-single_child_with_child_need_filter-0", - ) - assert "./index.html#STORY_2" in svg - for link in ( - "./index.html#STORY_1", - "./index.html#STORY_1.1", - "./index.html#STORY_1.2", - "./index.html#STORY_2.3", - "./index.html#SPEC_1", - "./index.html#SPEC_2", - "./index.html#SPEC_3", - "./index.html#SPEC_4", - "./index.html#STORY_3", - "./index.html#SPEC_5", - ): - assert link not in svg - - svg = _get_svg( - app.config, - outdir, - "single_child_need_filter.html", - "needflow-single_child_need_filter-0", - ) - assert "./index.html#SPEC_1" in svg - for link in ( - "./index.html#STORY_1", - "./index.html#STORY_1.1", - "./index.html#STORY_1.2", - "./index.html#STORY_2", - "./index.html#STORY_2.3", - "./index.html#SPEC_2", - "./index.html#SPEC_3", - "./index.html#SPEC_4", - "./index.html#STORY_3", - "./index.html#SPEC_5", - ): - assert link not in svg - - svg = _get_svg( - app.config, outdir, "grandy_and_child.html", "needflow-grandy_and_child-0" - ) - for link in ("./index.html#STORY_1", "./index.html#SPEC_1", "./index.html#SPEC_2"): - assert link in svg - for link in ( - "./index.html#STORY_1.1", - "./index.html#STORY_1.2", - "./index.html#STORY_2", - "./index.html#STORY_2.3", - "./index.html#SPEC_3", - "./index.html#SPEC_4", - "./index.html#STORY_3", - "./index.html#SPEC_5", - ): - assert link not in svg + if test_app.config.needs_flow_engine == "graphviz" and version_info < (7, 2): + pass # links will be wrong due to https://github.com/sphinx-doc/sphinx/pull/11078 + else: + for link in ( + '"../index.html#STORY_1"', + '"../index.html#STORY_1.1"', + '"../index.html#STORY_1.2"', + '"../index.html#STORY_2"', + '"../index.html#STORY_2.3"', + '"../index.html#SPEC_1"', + '"../index.html#SPEC_2"', + '"../index.html#SPEC_3"', + '"../index.html#SPEC_4"', + '"../index.html#STORY_3"', + '"../index.html#SPEC_5"', + ): + assert link in svg + + svg = _get_svg( + app.config, + outdir, + "single_parent_need_filer.html", + "needflow-single_parent_need_filer-0", + ) + + assert '"../index.html#STORY_3"' in svg + for link in ( + '"../index.html#STORY_1"', + '"../index.html#STORY_1.1"', + '"../index.html#STORY_1.2"', + '"../index.html#STORY_2"', + '"../index.html#STORY_2.3"', + '"../index.html#SPEC_1"', + '"../index.html#SPEC_2"', + '"../index.html#SPEC_3"', + '"../index.html#SPEC_4"', + '"../index.html#SPEC_5"', + ): + assert link not in svg + + svg = _get_svg( + app.config, + outdir, + "single_child_with_child_need_filter.html", + "needflow-single_child_with_child_need_filter-0", + ) + + assert '"../index.html#STORY_2"' in svg + for link in ( + '"../index.html#STORY_1"', + '"../index.html#STORY_1.1"', + '"../index.html#STORY_1.2"', + '"../index.html#STORY_2.3"', + '"../index.html#SPEC_1"', + '"../index.html#SPEC_2"', + '"../index.html#SPEC_3"', + '"../index.html#SPEC_4"', + '"../index.html#STORY_3"', + '"../index.html#SPEC_5"', + ): + assert link not in svg + + svg = _get_svg( + app.config, + outdir, + "single_child_need_filter.html", + "needflow-single_child_need_filter-0", + ) + assert '"../index.html#SPEC_1"' in svg + for link in ( + '"../index.html#STORY_1"', + '"../index.html#STORY_1.1"', + '"../index.html#STORY_1.2"', + '"../index.html#STORY_2"', + '"../index.html#STORY_2.3"', + '"../index.html#SPEC_2"', + '"../index.html#SPEC_3"', + '"../index.html#SPEC_4"', + '"../index.html#STORY_3"', + '"../index.html#SPEC_5"', + ): + assert link not in svg + + svg = _get_svg( + app.config, outdir, "grandy_and_child.html", "needflow-grandy_and_child-0" + ) + for link in ( + '"../index.html#STORY_1"', + '"../index.html#SPEC_1"', + '"../index.html#SPEC_2"', + ): + assert link in svg + for link in ( + '"../index.html#STORY_1.1"', + '"../index.html#STORY_1.2"', + '"../index.html#STORY_2"', + '"../index.html#STORY_2.3"', + '"../index.html#SPEC_3"', + '"../index.html#SPEC_4"', + '"../index.html#STORY_3"', + '"../index.html#SPEC_5"', + ): + assert link not in svg