From 58fee42172bc48dd536bf6d6411e0f091f144513 Mon Sep 17 00:00:00 2001 From: Chris Sewell Date: Wed, 28 Aug 2024 07:31:20 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fix=20rendering=20of=20`needextr?= =?UTF-8?q?act`=20needs=20(#1249)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes two key issues of `needextract` nodes: 1. The call to `BuildEnvironment.resolve_references` was already converting the need node to its final representation (since the calls `process_need_nodes`), and so the later call to `build_need` didn't do anything. This is why the requested layout was not being created. 2. A `needextract` can generate multiple need representation, but all of them were being given the same `id` of the original `NeedExtract` node. All nodes are now wrapped in a containing node, that is set with this `id` Additionally, warnings are now emitted for unfound needs, rather than raise exceptions Also, `build_need` is renamed to `build_need_repr`, to better describe its purpose, and its signature is slightly changed, to allow for the new logic. --- docs/directives/needextract.rst | 13 ++- sphinx_needs/directives/need.py | 11 +- sphinx_needs/directives/needextract.py | 153 +++++++++++-------------- sphinx_needs/layout.py | 68 ++++++----- 4 files changed, 114 insertions(+), 131 deletions(-) diff --git a/docs/directives/needextract.rst b/docs/directives/needextract.rst index 5073a30a8..2b28e21ba 100644 --- a/docs/directives/needextract.rst +++ b/docs/directives/needextract.rst @@ -23,12 +23,17 @@ For instance, a supplier could get a copy of requirements but would not see all :style: green_border -.. note:: +.. caution:: + + ``needextract`` supports correct rendering of most, but not all, content coming from the original need. + Be careful when using complex content in the original need, like custom roles or directives + that require any sphinx transforms. - **needextract** supports the full filtering possibilities of **Sphinx-Needs**. - Please read :ref:`filter` for more information. +``needextract`` supports the full filtering possibilities of sphinx-needs. +Please read :ref:`filter` for more information. -``needextract`` supports also arguments as filter string. It works like the option `filter`, but also +It also supports arguments as filter string, +which works like the option `filter`, but also supports need ID as filter argument. .. need-example:: diff --git a/sphinx_needs/directives/need.py b/sphinx_needs/directives/need.py index c18c86655..568c97b62 100644 --- a/sphinx_needs/directives/need.py +++ b/sphinx_needs/directives/need.py @@ -25,7 +25,7 @@ resolve_variants_options, ) from sphinx_needs.functions.functions import check_and_get_content -from sphinx_needs.layout import build_need +from sphinx_needs.layout import build_need_repr from sphinx_needs.logging import get_logger, log_warning from sphinx_needs.need_constraints import process_constraints from sphinx_needs.nodes import Need @@ -434,15 +434,18 @@ def format_need_nodes( need_id = node_need.attributes["ids"][0] need_data = needs[need_id] + if need_data["hide"]: + remove_node_from_tree(node_need) + continue + find_and_replace_node_content(node_need, env, need_data) for index, attribute in enumerate(node_need.attributes["classes"]): node_need.attributes["classes"][index] = check_and_get_content( attribute, need_data, env ) - layout = need_data["layout"] or NeedsSphinxConfig(app.config).default_layout - - build_need(layout, node_need, app, fromdocname=fromdocname) + rendered_node = build_need_repr(node_need, need_data, app, docname=fromdocname) + node_need.parent.replace(node_need, rendered_node) def check_links(needs: dict[str, NeedsInfoType], config: NeedsSphinxConfig) -> None: diff --git a/sphinx_needs/directives/needextract.py b/sphinx_needs/directives/needextract.py index 42ba67725..5742cf336 100644 --- a/sphinx_needs/directives/needextract.py +++ b/sphinx_needs/directives/needextract.py @@ -1,26 +1,29 @@ from __future__ import annotations import re -from typing import Sequence, cast +from typing import Sequence from docutils import nodes from docutils.parsers.rst import directives from docutils.transforms.references import Substitutions from sphinx.application import Sphinx from sphinx.environment.collectors.asset import DownloadFileCollector, ImageCollector +from sphinx.util.logging import getLogger -from sphinx_needs.api.exceptions import NeedsInvalidFilter from sphinx_needs.config import NeedsSphinxConfig -from sphinx_needs.data import NeedsExtractType, SphinxNeedsData +from sphinx_needs.data import NeedsExtractType, NeedsInfoType, SphinxNeedsData from sphinx_needs.debug import measure_time from sphinx_needs.directives.utils import ( no_needs_found_paragraph, used_filter_paragraph, ) from sphinx_needs.filter_common import FilterBase, process_filters -from sphinx_needs.layout import SphinxNeedLayoutException, build_need +from sphinx_needs.layout import build_need_repr +from sphinx_needs.logging import log_warning from sphinx_needs.utils import add_doc, remove_node_from_tree +LOGGER = getLogger(__name__) + class Needextract(nodes.General, nodes.Element): pass @@ -82,30 +85,42 @@ def process_needextract( env = app.env needs_config = NeedsSphinxConfig(app.config) - for node in found_nodes: + node: Needextract + for node in found_nodes: # type: ignore[assignment] if not needs_config.include_needs: remove_node_from_tree(node) continue current_needextract: NeedsExtractType = node.attributes all_needs = SphinxNeedsData(env).get_or_create_needs() - content: list[nodes.Element] = [] + content = nodes.container() + content.attributes["ids"] = [current_needextract["target_id"]] # check if filter argument and option filter both exist need_filter_arg = current_needextract["filter_arg"] if need_filter_arg and current_needextract["filter"]: - raise NeedsInvalidFilter( - "Needextract can't have filter arguments and option filter at the same time." + log_warning( + LOGGER, + "filter arguments and option filter at the same time are disallowed.", + "needextract", + location=node, ) + remove_node_from_tree(node) + continue elif need_filter_arg: # check if given filter argument is need-id if need_filter_arg in all_needs: need_filter_arg = f'id == "{need_filter_arg}"' elif re.fullmatch(needs_config.id_regex, need_filter_arg): # check if given filter argument is need-id, but not exists - raise NeedsInvalidFilter( - f"Provided id {need_filter_arg} for needextract does not exist." + log_warning( + LOGGER, + f"Requested need {need_filter_arg!r} not found.", + "needextract", + location=node, ) + remove_node_from_tree(node) + continue current_needextract["filter"] = need_filter_arg found_needs = process_filters( @@ -119,18 +134,15 @@ def process_needextract( for need_info in found_needs: # filter out need_part from found_needs, in order to generate # copies of filtered needs with custom layout and style - if need_info["is_need"] and not need_info["is_part"]: - need_extract = _build_needextract( - need_info["id"], - app, - layout=current_needextract["layout"], - style=current_needextract["style"], - docname=current_needextract["docname"], + if ( + need_info["is_need"] + and not need_info["is_part"] + and ( + need_extract := _build_needextract( + app, node, need_info, current_needextract + ) ) - - # Add lineno to node - need_extract.line = current_needextract["lineno"] - + ): content.append(need_extract) if len(content) == 0: @@ -152,87 +164,54 @@ def process_needextract( @measure_time("build_needextract") def _build_needextract( - need_id: str, app: Sphinx, - layout: str | None = None, - style: str | None = None, - docname: str | None = None, -) -> nodes.container: - """ - Creates a new need-node for a given layout. - - Need must already exist in internal dictionary. - This creates a new representation only. - :param need_id: need id - :param app: sphinx application - :param layout: layout to use, overrides layout set by need itself - :param style: style to use, overrides styles set by need itself - :param docname: Needed for calculating references - :return: - """ + extract_node: Needextract, + need_data: NeedsInfoType, + extract_data: NeedsExtractType, +) -> nodes.container | None: + """Creates a new need representation.""" env = app.env - needs = SphinxNeedsData(env).get_or_create_needs() - - if need_id not in needs.keys(): - raise SphinxNeedLayoutException(f"Given need id {need_id} does not exist.") - - need_data = needs[need_id] - - # Resolve internal references. - # This is done for original need content automatically. - # But as we are working on a copy, we have to trigger this on our own. - if docname is None: - # needed to calculate relative references - # TODO ideally we should not cast here: - # the docname can still be None, if the need is external, although practically these are not rendered - docname = cast(str, needs[need_id]["docname"]) - node_container = nodes.container() - # node_container += needs[need_id]["need_node"].children - - node_inner = SphinxNeedsData(env).get_need_node(need_id) - assert node_inner is not None, f"Need {need_id} has no content node." + if (need_node := SphinxNeedsData(env).get_need_node(need_data["id"])) is None: + log_warning( + LOGGER, + f"Content for requested need {need_data['id']!r} not found.", + "needextract", + location=extract_node, + ) + return None - # Rerun some important Sphinx collectors for need-content coming from "needsexternal". - # This is needed, as Sphinx needs to know images and download paths. - # Normally this gets done much earlier in the process, so that for the copied need-content this - # handling was and will not be done by Sphinx itself anymore. + dummy_need = nodes.container() + dummy_need.source, dummy_need.line = extract_node.source, extract_node.line - # Overwrite the docname, which must be the original one from the reused need, as all used paths are relative - # to the original location, not to the current document. + # Try to implement Sphinx transforms that would have already been done if the need was in the original document. + # Note, this is very hacky and can not possibly account for all transforms. env.temp_data["docname"] = need_data[ "docname" - ] # Dirty, as in this phase normally no docname is set anymore in env - ImageCollector().process_doc(app, node_inner) # type: ignore[arg-type] - DownloadFileCollector().process_doc(app, node_inner) # type: ignore[arg-type] - + ] # this is normally set in the read phase + ImageCollector().process_doc(app, need_node) # type: ignore[arg-type] + DownloadFileCollector().process_doc(app, need_node) # type: ignore[arg-type] del env.temp_data["docname"] # Be sure our env is as it was before - node_container.append(node_inner) + dummy_need.extend(need_node.children) # resolve_references() ignores the given docname and takes the docname from the pending_xref node. # Therefore, we need to manipulate this first, before we can ask Sphinx to perform the normal # reference handling for us. - _replace_pending_xref_refdoc(node_container, docname) - env.resolve_references(node_container, docname, env.app.builder) # type: ignore[arg-type] - - node_container.attributes["ids"].append(need_id) - - needs_config = NeedsSphinxConfig(app.config) - layout = layout or need_data["layout"] or needs_config.default_layout - style = style or need_data["style"] or needs_config.default_style - - build_need(layout, node_container, app, style, docname) - - # set the layout and style for the new need - node_container[0].attributes = node_container.parent.children[0].attributes # type: ignore - node_container[0].children[0].attributes = ( # type: ignore - node_container.parent.children[0].children[0].attributes # type: ignore + _replace_pending_xref_refdoc(dummy_need, extract_data["docname"]) + env.resolve_references(dummy_need, extract_data["docname"], app.builder) # type: ignore[arg-type] + + dummy_need.attributes["ids"].append(need_data["id"]) + rendered_node = build_need_repr( + dummy_need, # type: ignore[arg-type] + need_data, + app, + layout=extract_data["layout"], + style=extract_data["style"], + docname=extract_data["docname"], ) - node_container.attributes["ids"] = [] - - return node_container + return rendered_node def _replace_pending_xref_refdoc(node: nodes.Element, new_refdoc: str) -> None: diff --git a/sphinx_needs/layout.py b/sphinx_needs/layout.py index 2e12561f7..beb54ce40 100644 --- a/sphinx_needs/layout.py +++ b/sphinx_needs/layout.py @@ -27,26 +27,32 @@ from sphinx.util.logging import getLogger from sphinx_needs.config import NeedsSphinxConfig -from sphinx_needs.data import NeedsCoreFields, NeedsInfoType, SphinxNeedsData +from sphinx_needs.data import NeedsCoreFields, NeedsInfoType from sphinx_needs.debug import measure_time from sphinx_needs.logging import log_warning +from sphinx_needs.nodes import Need from sphinx_needs.utils import match_string_link LOGGER = getLogger(__name__) -@measure_time("need") -def build_need( - layout: str, - node: nodes.Element, +@measure_time("build_need_repr") +def build_need_repr( + node: Need, + data: NeedsInfoType, app: Sphinx, + *, + layout: str | None = None, style: str | None = None, - fromdocname: str | None = None, -) -> None: - """ - Builds a need based on a given layout for a given need-node. + docname: str | None = None, +) -> nodes.container: + """Create an output representation for a need. + + :param layout: Override layout from need data / config + :param style: Override style from need data / config + :param docname: Override docname from need data / config - The created table must have the following docutils structure:: + The created table will have the following docutils structure:: - table -- tgroup @@ -59,23 +65,9 @@ def build_need( The level structure must be kept, otherwise docutils can not handle it! """ - - env = app.env - needs = SphinxNeedsData(env).get_or_create_needs() node_container = nodes.container() - need_id = node.attributes["ids"][0] - need_data = needs[need_id] - - if need_data["hide"]: - if node.parent: - node.parent.replace(node, []) - return - - if fromdocname is None: - fromdocname = need_data["docname"] - - lh = LayoutHandler(app, need_data, layout, node, style, fromdocname) + lh = LayoutHandler(app, data, node, layout=layout, style=style, docname=docname) new_need_node = lh.get_need_table() node_container.append(new_need_node) @@ -83,9 +75,7 @@ def build_need( node_container.attributes["ids"] = [container_id] node_container.attributes["classes"] = ["need_container"] - # We need to replace the current need-node (containing content only) with our new table need node. - # node.parent.replace(node, node_container) - node.parent.replace(node, node_container) + return node_container @lru_cache(1) @@ -105,16 +95,25 @@ def __init__( self, app: Sphinx, need: NeedsInfoType, - layout: str, - node: nodes.Element, + node: Need, + *, + layout: str | None = None, style: str | None = None, - fromdocname: str | None = None, + docname: str | None = None, ) -> None: + """ + + :param layout: Override layout from need data / config + :param style: Override style from need data / config + :param docname: Override docname from need data / config + """ self.app = app self.need = need self.needs_config = NeedsSphinxConfig(app.config) - self.layout_name = layout + self.layout_name = ( + layout or self.need["layout"] or self.needs_config.default_layout + ) available_layouts = self.needs_config.layouts if self.layout_name not in available_layouts: raise SphinxNeedLayoutException( @@ -127,10 +126,7 @@ def __init__( self.node = node # Used, if you need is referenced from another page - if fromdocname is None: - self.fromdocname = need["docname"] - else: - self.fromdocname = fromdocname + self.fromdocname = need["docname"] if docname is None else docname classes = [ "need",