From 42e9f2b88d43231b4a43efca865576b9bd936bb8 Mon Sep 17 00:00:00 2001 From: Chris Sewell Date: Sun, 12 Jul 2026 13:02:58 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20FIX:=20Ignore=20comments/targets?= =?UTF-8?q?=20in=20component=20child=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comments, hyperlink targets, and system messages are structurally inert, so they are now silently tolerated as children of the grid, card-carousel, tab-set, and tab-set-code directives, rather than emitting a confusing "All children of X should be Y" warning. Hyperlink targets inside a tab-set are additionally preserved, both when the directive rebuilds its children and when TabSetHtmlTransform restructures the tab-set for HTML output (targets are moved to the front of the rebuilt node), so that references to them still resolve. Closes #86 --- sphinx_design/cards.py | 3 + sphinx_design/grids.py | 3 + sphinx_design/shared.py | 9 ++ sphinx_design/tabs.py | 35 +++++- tests/test_misc.py | 270 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 314 insertions(+), 6 deletions(-) diff --git a/sphinx_design/cards.py b/sphinx_design/cards.py index c243d90..e9abc7f 100644 --- a/sphinx_design/cards.py +++ b/sphinx_design/cards.py @@ -16,6 +16,7 @@ SdDirective, create_component, is_component, + is_ignorable_child, make_choice, margin_option, text_align, @@ -284,6 +285,8 @@ def run_with_defaults(self) -> list[nodes.Node]: self.set_source_info(container) self.state.nested_parse(self.content, self.content_offset, container) for item in container.children: + if is_ignorable_child(item): + continue if not is_component(item, "card"): LOGGER.warning( "All children of a 'card-carousel' " diff --git a/sphinx_design/grids.py b/sphinx_design/grids.py index 1610c0c..743f231 100644 --- a/sphinx_design/grids.py +++ b/sphinx_design/grids.py @@ -9,6 +9,7 @@ SdDirective, create_component, is_component, + is_ignorable_child, make_choice, margin_option, padding_option, @@ -142,6 +143,8 @@ def run_with_defaults(self) -> list[nodes.Node]: self.state.nested_parse(self.content, self.content_offset, row) # each item in a row should be a column for item in row.children: + if is_ignorable_child(item): + continue if not is_component(item, "grid-item"): LOGGER.warning( f"All children of a 'grid-row' " diff --git a/sphinx_design/shared.py b/sphinx_design/shared.py index d10f895..acdf3eb 100644 --- a/sphinx_design/shared.py +++ b/sphinx_design/shared.py @@ -143,6 +143,15 @@ def is_component(node: nodes.Node, name: str): return False +#: Node types that are structurally inert inside component containers. +SKIP_CHILD_TYPES = (nodes.comment, nodes.target, nodes.system_message) + + +def is_ignorable_child(node: nodes.Node) -> bool: + """Check if a node can be silently ignored when validating component children.""" + return isinstance(node, SKIP_CHILD_TYPES) + + def make_choice(choices: Sequence[str]): """Create a choice validator.""" return lambda argument: directives.choice(argument, choices) diff --git a/sphinx_design/tabs.py b/sphinx_design/tabs.py index 0edd6df..03ba1e5 100644 --- a/sphinx_design/tabs.py +++ b/sphinx_design/tabs.py @@ -7,7 +7,13 @@ from sphinx.util.logging import getLogger from ._compat import findall -from .shared import WARNING_TYPE, SdDirective, create_component, is_component +from .shared import ( + WARNING_TYPE, + SdDirective, + create_component, + is_component, + is_ignorable_child, +) LOGGER = getLogger(__name__) @@ -39,6 +45,13 @@ def run_with_defaults(self) -> list[nodes.Node]: self.state.nested_parse(self.content, self.content_offset, tab_set) valid_children = [] for item in tab_set.children: + if is_ignorable_child(item): + # comments and system messages can be safely dropped, + # but hyperlink targets must be kept, + # so that references to them still resolve + if isinstance(item, nodes.target): + valid_children.append(item) + continue if not is_component(item, "tab-item"): LOGGER.warning( f"All children of a 'tab-set' " @@ -145,6 +158,8 @@ def run_with_defaults(self) -> list[nodes.Node]: self.state.nested_parse(self.content, self.content_offset, tab_set) new_children = [] for item in tab_set.children: + if is_ignorable_child(item): + continue if not isinstance(item, nodes.literal_block): LOGGER.warning( f"All children of a 'tab-code-set' " @@ -230,9 +245,19 @@ def run(self, **kwargs: Any) -> None: ): tab_set_identity = tab_set_id_base + str(tab_set_id_num) children = [] + # keep non tab-item children (e.g. hyperlink targets), + # placing them at the front of the rebuilt tab-set + preserved = [ + child + for child in tab_set.children + if not is_component(child, "tab-item") + ] + tab_items = [ + child for child in tab_set.children if is_component(child, "tab-item") + ] # get the first selected node selected_idx = None - for idx, tab_item in enumerate(tab_set.children): + for idx, tab_item in enumerate(tab_items): if tab_item.get("selected", False): if selected_idx is None: selected_idx = idx @@ -245,9 +270,7 @@ def run(self, **kwargs: Any) -> None: ) selected_idx = 0 if selected_idx is None else selected_idx - for idx, tab_item in enumerate(tab_set.children): - if not is_component(tab_item, "tab-item"): - continue # Skip non tab-item children + for idx, tab_item in enumerate(tab_items): if len(tab_item.children) != 2: LOGGER.warning( f"Malformed 'tab-item' directive [{WARNING_TYPE}.tab]", @@ -290,4 +313,4 @@ def run(self, **kwargs: Any) -> None: # add content children.append(tab_content) - tab_set.children = children + tab_set.children = preserved + children diff --git a/tests/test_misc.py b/tests/test_misc.py index 9266770..77fceea 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1,6 +1,10 @@ +from docutils import nodes import pytest +from sphinx_design._compat import findall from sphinx_design.icons import get_material_icon_data, get_octicon_data +from sphinx_design.shared import is_component +from sphinx_design.tabs import sd_tab_input def test_octicons(file_regression): @@ -72,3 +76,269 @@ def test_tab_set_with_invalid_children( extension=".xml", encoding="utf8", ) + + +GRID_WITH_COMMENT = { + "rst": """ +Test +==== + +.. grid:: 2 + + .. a comment + + .. grid-item:: + + A + + .. grid-item:: + + B +""", + "myst": """ +# Test + +`````{grid} 2 + +% a comment + +```{grid-item} +A +``` + +```{grid-item} +B +``` +````` +""", +} + + +@pytest.mark.parametrize("fmt", ["rst", "myst"]) +def test_grid_with_comment(fmt, sphinx_builder): + """A comment between grid-items should not trigger a warning. + + See https://github.com/executablebooks/sphinx-design/issues/86 + """ + if fmt == "rst": + builder = sphinx_builder(conf_kwargs={"extensions": ["sphinx_design"]}) + builder.src_path.joinpath("index.rst").write_text( + GRID_WITH_COMMENT["rst"], encoding="utf8" + ) + else: + builder = sphinx_builder() + builder.src_path.joinpath("index.md").write_text( + GRID_WITH_COMMENT["myst"], encoding="utf8" + ) + builder.build() # asserts no warnings + doctree = builder.get_doctree("index") + grid_items = list(findall(doctree)(lambda node: is_component(node, "grid-item"))) + assert len(grid_items) == 2 + + +CARD_CAROUSEL_WITH_COMMENT = { + "rst": """ +Test +==== + +.. card-carousel:: 2 + + .. a comment + + .. card:: Card A + + content A + + .. card:: Card B + + content B +""", + "myst": """ +# Test + +`````{card-carousel} 2 + +% a comment + +````{card} Card A +content A +```` + +````{card} Card B +content B +```` +````` +""", +} + + +@pytest.mark.parametrize("fmt", ["rst", "myst"]) +def test_card_carousel_with_comment(fmt, sphinx_builder): + """A comment between cards should not trigger a warning. + + See https://github.com/executablebooks/sphinx-design/issues/86 + """ + if fmt == "rst": + builder = sphinx_builder(conf_kwargs={"extensions": ["sphinx_design"]}) + builder.src_path.joinpath("index.rst").write_text( + CARD_CAROUSEL_WITH_COMMENT["rst"], encoding="utf8" + ) + else: + builder = sphinx_builder() + builder.src_path.joinpath("index.md").write_text( + CARD_CAROUSEL_WITH_COMMENT["myst"], encoding="utf8" + ) + builder.build() # asserts no warnings + doctree = builder.get_doctree("index") + cards = list(findall(doctree)(lambda node: is_component(node, "card"))) + assert len(cards) == 2 + + +TAB_SET_WITH_COMMENT = { + "rst": """ +Test +==== + +.. tab-set:: + + .. a comment + + .. tab-item:: A + + A content + + .. tab-item:: B + + B content +""", + "myst": """ +# Test + +````{tab-set} + +% a comment + +```{tab-item} A +A content +``` + +```{tab-item} B +B content +``` +```` +""", +} + + +@pytest.mark.parametrize("fmt", ["rst", "myst"]) +def test_tab_set_with_comment(fmt, sphinx_builder): + """A comment between tab-items should not trigger a warning. + + See https://github.com/executablebooks/sphinx-design/issues/86 + """ + if fmt == "rst": + builder = sphinx_builder(conf_kwargs={"extensions": ["sphinx_design"]}) + builder.src_path.joinpath("index.rst").write_text( + TAB_SET_WITH_COMMENT["rst"], encoding="utf8" + ) + else: + builder = sphinx_builder() + builder.src_path.joinpath("index.md").write_text( + TAB_SET_WITH_COMMENT["myst"], encoding="utf8" + ) + builder.build() # asserts no warnings + # both tabs should still render in the HTML output + doctree = builder.get_doctree("index", post_transforms=True) + inputs = list(findall(doctree)(sd_tab_input)) + assert len(inputs) == 2 + assert inputs[0]["checked"] is True + + +TAB_SET_WITH_TARGET = { + "rst": """ +Test +==== + +.. tab-set:: + + .. _my-target: + + .. tab-item:: A + + A content + +See :ref:`target link `. +""", + "myst": """ +# Test + +````{tab-set} + +(my-target)= + +```{tab-item} A +A content +``` +```` + +See {ref}`target link `. +""", +} + + +@pytest.mark.parametrize("fmt", ["rst", "myst"]) +def test_tab_set_with_target(fmt, sphinx_builder): + """A hyperlink target before a tab-item should be preserved, + so that references to it still resolve, and should not trigger a warning. + + See https://github.com/executablebooks/sphinx-design/issues/86 + """ + if fmt == "rst": + builder = sphinx_builder(conf_kwargs={"extensions": ["sphinx_design"]}) + builder.src_path.joinpath("index.rst").write_text( + TAB_SET_WITH_TARGET["rst"], encoding="utf8" + ) + else: + builder = sphinx_builder() + builder.src_path.joinpath("index.md").write_text( + TAB_SET_WITH_TARGET["myst"], encoding="utf8" + ) + builder.build() # asserts no warnings + doctree = builder.get_doctree("index", post_transforms=True) + # the target survives both the directive and HTML transform rebuilds, + # placed at the front of the tab-set + targets = list(findall(doctree)(nodes.target)) + assert len(targets) == 1 + assert is_component(targets[0].parent, "tab-set") + assert targets[0].parent.children[0] is targets[0] + # the tab still renders and is selected by default + inputs = list(findall(doctree)(sd_tab_input)) + assert len(inputs) == 1 + assert inputs[0]["checked"] is True + # the reference resolves to the target + references = list(findall(doctree)(nodes.reference)) + assert len(references) == 1 + assert references[0]["refid"] == "my-target" + + +def test_tab_set_with_paragraph_warns(sphinx_builder): + """A bare paragraph directly inside a tab-set should still warn.""" + builder = sphinx_builder(conf_kwargs={"extensions": ["sphinx_design"]}) + builder.src_path.joinpath("index.rst").write_text( + """ +Test +==== + +.. tab-set:: + + .. tab-item:: A + + A content + + not a tab item +""", + encoding="utf8", + ) + builder.build(assert_pass=False) + assert "All children of a 'tab-set' should be 'tab-item'" in builder.warnings + assert "[design.tab]" in builder.warnings