From ff4ae90378952c673945c7a6a8eb3b4eb0070183 Mon Sep 17 00:00:00 2001 From: Chris Sewell Date: Wed, 21 Aug 2024 08:31:19 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix=20warnings=20for=20duplicate?= =?UTF-8?q?=20needs=20in=20parallel=20builds=20(#1223)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change duplicate need feedback from raising exceptions to emitting Sphinx warnings, e.g. ``` path/to/page.rst:4: WARNING: A need with ID STORY_PAGE_1 already exists, title: 'duplicate'. [needs.duplicate_id] ``` and ensure warning is also emitted during parallel builds Note, although there was already a parallel build test in the test suite, actually it was not running parallel because there were not enough documents in the test project (see https://github.com/sphinx-doc/sphinx/pull/12796), so one more document is added. --- sphinx_needs/api/exceptions.py | 4 --- sphinx_needs/api/need.py | 20 ++++++++------ sphinx_needs/data.py | 34 +++++++++++++++++++---- sphinx_needs/external_needs.py | 13 +++++---- tests/doc_test/parallel_doc/index.rst | 1 + tests/doc_test/parallel_doc/page_5.rst | 5 ++++ tests/test_broken_doc.py | 30 +++++++++++++-------- tests/test_parallel_execution.py | 37 +++++++++++++++----------- 8 files changed, 96 insertions(+), 48 deletions(-) create mode 100644 tests/doc_test/parallel_doc/page_5.rst diff --git a/sphinx_needs/api/exceptions.py b/sphinx_needs/api/exceptions.py index 1720441aa..406e1e506 100644 --- a/sphinx_needs/api/exceptions.py +++ b/sphinx_needs/api/exceptions.py @@ -19,10 +19,6 @@ class NeedsNoIdException(SphinxError): pass -class NeedsDuplicatedId(SphinxError): - pass - - class NeedsStatusNotAllowed(SphinxError): pass diff --git a/sphinx_needs/api/need.py b/sphinx_needs/api/need.py index ccfe6598b..8b805443f 100644 --- a/sphinx_needs/api/need.py +++ b/sphinx_needs/api/need.py @@ -16,7 +16,6 @@ from sphinx_needs.api.configuration import NEEDS_CONFIG from sphinx_needs.api.exceptions import ( NeedsConstraintNotAllowed, - NeedsDuplicatedId, NeedsInvalidException, NeedsInvalidOption, NeedsNoIdException, @@ -312,19 +311,24 @@ def run(): if need_id in SphinxNeedsData(env).get_or_create_needs(): if id: - raise NeedsDuplicatedId( - f"A need with ID {need_id} already exists! " - f"This is not allowed. Document {docname}[{lineno}] Title: {title}." - ) + message = f"A need with ID {need_id} already exists, " f"title: {title!r}." else: # this is a generated ID - raise NeedsDuplicatedId( + _title = " ".join(title) + message = ( "Needs could not generate a unique ID for a need with " - "the title '{}' because another need had the same title. " + f"the title {_title!r} because another need had the same title. " "Either supply IDs for the requirements or ensure the " "titles are different. NOTE: If title is being generated " "from the content, then ensure the first sentence of the " - "requirements are different.".format(" ".join(title)) + "requirements are different." ) + logger.warning( + message + " [needs.duplicate_id]", + type="needs", + subtype="duplicate_id", + location=(docname, lineno) if docname else None, + ) + return [] # Trim title if it is too long max_length = needs_config.max_title_length diff --git a/sphinx_needs/data.py b/sphinx_needs/data.py index a97d76c60..b02ba3a5e 100644 --- a/sphinx_needs/data.py +++ b/sphinx_needs/data.py @@ -6,6 +6,8 @@ from typing import TYPE_CHECKING, Literal, TypedDict +from sphinx.util.logging import getLogger + if TYPE_CHECKING: from docutils.nodes import Element, Text from sphinx.application import Sphinx @@ -15,6 +17,9 @@ from sphinx_needs.services.manager import ServiceManager +LOGGER = getLogger(__name__) + + class NeedsFilterType(TypedDict): filter: str """Filter string.""" @@ -612,7 +617,7 @@ def get_or_create_umls(self) -> dict[str, NeedsUmlType]: def merge_data( - _app: Sphinx, env: BuildEnvironment, _docnames: list[str], other: BuildEnvironment + _app: Sphinx, env: BuildEnvironment, docnames: list[str], other: BuildEnvironment ) -> None: """ Performs data merge of parallel executed workers. @@ -621,13 +626,32 @@ def merge_data( Needs to update env manually for all data Sphinx-Needs collect during read phase """ - # Update global needs dict - needs = SphinxNeedsData(env).get_or_create_needs() - other_needs = SphinxNeedsData(other).get_or_create_needs() - needs.update(other_needs) if SphinxNeedsData(other).has_export_filters: SphinxNeedsData(env).has_export_filters = True + # Update needs + needs = SphinxNeedsData(env).get_or_create_needs() + other_needs = SphinxNeedsData(other).get_or_create_needs() + for other_id, other_need in other_needs.items(): + if other_id in needs: + # we only want to warn if the need comes from one of the docs parsed in this worker + _docname = other_need["docname"] + if _docname in docnames: + message = ( + f"A need with ID {other_id} already exists, " + f"title: {other_need['title']!r}." + ) + LOGGER.warning( + message + " [needs.duplicate_id]", + type="needs", + subtype="duplicate_id", + location=(_docname, other_need["lineno"]) if _docname else None, + ) + else: + needs[other_id] = other_need + + # update other data + def _merge(name: str, is_complex_dict: bool = False) -> None: # Update global needs dict if not hasattr(env, name): diff --git a/sphinx_needs/external_needs.py b/sphinx_needs/external_needs.py index eaddfe784..3ecc1f3fe 100644 --- a/sphinx_needs/external_needs.py +++ b/sphinx_needs/external_needs.py @@ -11,7 +11,6 @@ from sphinx.environment import BuildEnvironment from sphinx_needs.api import add_external_need, del_need -from sphinx_needs.api.exceptions import NeedsDuplicatedId from sphinx_needs.config import NeedsSphinxConfig from sphinx_needs.data import SphinxNeedsData from sphinx_needs.logging import get_logger @@ -30,7 +29,7 @@ def get_target_template(target_url: str) -> Template: return mem_template -def load_external_needs(app: Sphinx, env: BuildEnvironment, _docname: str) -> None: +def load_external_needs(app: Sphinx, env: BuildEnvironment, docname: str) -> None: """Load needs from configured external sources.""" needs_config = NeedsSphinxConfig(app.config) for source in needs_config.external_needs: @@ -159,10 +158,14 @@ def load_external_needs(app: Sphinx, env: BuildEnvironment, _docname: str) -> No # delete the already existing external need from api need del_need(app, ext_need_id) else: - raise NeedsDuplicatedId( - f'During external needs handling, an identical ID was detected: {ext_need_id} \ - from needs_external_needs url: {source["base_url"]}' + log.warning( + f'During external needs handling, an identical ID was detected: {ext_need_id} ' + f'from needs_external_needs url: {source["base_url"]} [needs.duplicate_id]', + type="needs", + subtype="duplicate_id", + location=docname if docname else None, ) + return None add_external_need(app, **need_params) diff --git a/tests/doc_test/parallel_doc/index.rst b/tests/doc_test/parallel_doc/index.rst index 4c318ed9f..8b29080b3 100644 --- a/tests/doc_test/parallel_doc/index.rst +++ b/tests/doc_test/parallel_doc/index.rst @@ -7,6 +7,7 @@ PARALLEL TEST DOCUMENT page_2 page_3 page_4 + page_5 .. spec:: Command line interface :id: SP_TOO_001 diff --git a/tests/doc_test/parallel_doc/page_5.rst b/tests/doc_test/parallel_doc/page_5.rst new file mode 100644 index 000000000..602d47c92 --- /dev/null +++ b/tests/doc_test/parallel_doc/page_5.rst @@ -0,0 +1,5 @@ +Page 5 +====== + +.. story:: duplicate + :id: STORY_PAGE_1 diff --git a/tests/test_broken_doc.py b/tests/test_broken_doc.py index d203541e0..1eddf7f69 100644 --- a/tests/test_broken_doc.py +++ b/tests/test_broken_doc.py @@ -1,18 +1,26 @@ -import pytest +import os -from sphinx_needs.api.need import NeedsDuplicatedId +import pytest +from sphinx.testing.util import SphinxTestApp +from sphinx.util.console import strip_colors @pytest.mark.parametrize( "test_app", - [{"buildername": "html", "srcdir": "doc_test/broken_doc"}], + [{"buildername": "html", "srcdir": "doc_test/broken_doc", "no_plantuml": True}], indirect=True, ) -def test_doc_build_html(test_app): - with pytest.raises(NeedsDuplicatedId): - app = test_app - - app.build() - html = (app.outdir / "index.html").read_text() - assert "

BROKEN DOCUMENT" in html - assert "SP_TOO_001" in html +def test_doc_build_html(test_app: SphinxTestApp): + test_app.build() + warnings = ( + strip_colors(test_app._warning.getvalue()) + .replace(str(test_app.srcdir) + os.path.sep, "/") + .strip() + ) + assert ( + warnings + == "/index.rst:11: WARNING: A need with ID SP_TOO_001 already exists, title: 'Command line interface'. [needs.duplicate_id]" + ) + html = (test_app.outdir / "index.html").read_text() + assert "

BROKEN DOCUMENT" in html + assert "SP_TOO_001" in html diff --git a/tests/test_parallel_execution.py b/tests/test_parallel_execution.py index 44164ffc3..9f2bf0422 100644 --- a/tests/test_parallel_execution.py +++ b/tests/test_parallel_execution.py @@ -1,31 +1,38 @@ +import os from pathlib import Path import pytest +from sphinx.util.console import strip_colors @pytest.mark.parametrize( "test_app", - [{"buildername": "html", "srcdir": "doc_test/parallel_doc", "parallel": 4}], + [ + { + "buildername": "html", + "srcdir": "doc_test/parallel_doc", + "parallel": 4, + "no_plantuml": True, + } + ], indirect=True, ) def test_doc_build_html(test_app): app = test_app app.build() - html = Path(app.outdir, "index.html").read_text() - assert app.statuscode == 0 - assert "

PARALLEL TEST DOCUMENT" in html - assert "SP_TOO_001" in html - + warnings = ( + strip_colors(app._warning.getvalue()) + .replace(str(app.srcdir) + os.path.sep, "/") + .strip() + ) + assert ( + warnings + == "/page_5.rst:4: WARNING: A need with ID STORY_PAGE_1 already exists, title: 'duplicate'. [needs.duplicate_id]" + ) -@pytest.mark.parametrize( - "test_app", - [{"buildername": "html", "srcdir": "doc_test/parallel_doc", "parallel": 4}], - indirect=True, -) -def test_doc_parallel_build_html(test_app): - app = test_app - app.build() - assert app.statuscode == 0 + index_html = Path(app.outdir, "index.html").read_text() + assert "

PARALLEL TEST DOCUMENT" in index_html + assert "SP_TOO_001" in index_html page3_html = Path(app.outdir, "page_3.html").read_text() assert "Page 3" in page3_html