Skip to content

Commit

Permalink
🐛 FIX needflow graphviz links (#1339)
Browse files Browse the repository at this point in the history 
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: sphinx-doc/sphinx#11078)
  • Loading branch information
@chrisjsewell
chrisjsewell authored Oct 28, 2024
1 parent 7a3e9ef commit 8aa192b
Show file tree
Hide file tree
Showing 2 changed files with 170 additions and 121 deletions.
36 changes: 31 additions & 5 deletions sphinx_needs/directives/needflow/_graphviz.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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,
)
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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,
Expand Down
255 changes: 139 additions & 116 deletions tests/test_needflow.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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):
Expand All @@ -50,40 +52,50 @@ 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,
outdir,
"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

Expand Down Expand Up @@ -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):
Expand All @@ -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

0 comments on commit 8aa192b

Please sign in to comment.