Repository: useblocks/sphinx-needs
Pull request #1224 (2024-08-21 04:26:16)

🔧 Deprecate unused `add_need` kwargs
Currently they are silently ignored,
so this commit specifically checks for them and provides a warning. This can then be removed in a future version of sphinx-needs

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1224?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) Report
Attention: Patch coverage is `60.00000%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 86.42%. Comparing base [(`2045121`)](https://app.codecov.io/gh/useblocks/sphinx-needs/commit/20451216479b46272121d8ddd7c43a78824aba72?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) to head [(`c661413`)](https://app.codecov.io/gh/useblocks/sphinx-needs/commit/c661413b0c37353540b7a774f758c838026988ed?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks).

| [Files](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1224?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) | Patch % | Lines |
|---|---|---|
| [sphinx\_needs/api/need.py](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1224?src=pr&el=tree&filepath=sphinx_needs%2Fapi%2Fneed.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks#diff-c3BoaW54X25lZWRzL2FwaS9uZWVkLnB5) | 60.00% | [2 Missing :warning: ](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1224?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #1224      +/-   ##
==========================================
- Coverage   86.44%   86.42%   -0.03%     
==========================================
  Files          56       56              
  Lines        6501     6506       +5     
==========================================
+ Hits         5620     5623       +3     
- Misses        881      883       +2     
```

| [Flag](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1224/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) | Coverage Δ | |
|---|---|---|
| [pytests](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1224/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) | `86.42% <60.00%> (-0.03%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks#carryforward-flags-in-the-pull-request-comment) to find out more.

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1224?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks).


Patch:
diff --git a/sphinx_needs/api/exceptions.py b/sphinx_needs/api/exceptions.py
index 1720441a..406e1e50 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 ccfe6598..d9c88ce2 100644
--- a/sphinx_needs/api/need.py
+++ b/sphinx_needs/api/need.py
@@ -3,6 +3,7 @@ from __future__ import annotations
 import hashlib
 import os
 import re
+import warnings
 from contextlib import contextmanager
 from typing import Any, Iterator
 
@@ -16,7 +17,6 @@ from sphinx.environment import BuildEnvironment
 from sphinx_needs.api.configuration import NEEDS_CONFIG
 from sphinx_needs.api.exceptions import (
     NeedsConstraintNotAllowed,
-    NeedsDuplicatedId,
     NeedsInvalidException,
     NeedsInvalidOption,
     NeedsNoIdException,
@@ -35,6 +35,8 @@ from sphinx_needs.utils import jinja_parse
 
 logger = get_logger(__name__)
 
+_deprecated_kwargs = {"constraints_passed", "links_string", "hide_tags", "hide_status"}
+
 
 def add_need(
     app: Sphinx,
@@ -50,13 +52,9 @@ def add_need(
     status: str | None = None,
     tags: None | str | list[str] = None,
     constraints: None | str | list[str] = None,
-    constraints_passed: None | bool = None,
-    links_string: None | str | list[str] = None,
     delete: None | bool = False,
     jinja_content: None | bool = False,
     hide: bool = False,
-    hide_tags: bool = False,
-    hide_status: bool = False,
     collapse: None | bool = None,
     style: None | str = None,
     layout: None | str = None,
@@ -136,11 +134,8 @@ def add_need(
     :param tags: Tags as single string.
     :param constraints: Constraints as single, comma separated, string.
     :param constraints_passed: Contains bool describing if all constraints have passed
-    :param links_string: Links as single string. (Not used)
     :param delete: boolean value (Remove the complete need).
     :param hide: boolean value.
-    :param hide_tags: boolean value. (Not used with Sphinx-Needs >0.5.0)
-    :param hide_status: boolean value. (Not used with Sphinx-Needs >0.5.0)
     :param collapse: boolean value.
     :param style: String value of class attribute of node.
     :param layout: String value of layout definition to use
@@ -150,6 +145,13 @@ def add_need(
 
     :return: node
     """
+
+    if kwargs.keys() & _deprecated_kwargs:
+        warnings.warn(
+            "deprecated key found in kwargs", DeprecationWarning, stacklevel=1
+        )
+        kwargs = {k: v for k, v in kwargs.items() if k not in _deprecated_kwargs}
+
     #############################################################################################
     # Get environment
     #############################################################################################
@@ -312,19 +314,24 @@ def add_need(
 
     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
@@ -620,7 +627,6 @@ def add_external_need(
     status: str | None = None,
     tags: str | None = None,
     constraints: str | None = None,
-    links_string: str | None = None,
     **kwargs: Any,
 ) -> list[nodes.Node]:
     """
@@ -640,7 +646,6 @@ def add_external_need(
     :param status: Status as string.
     :param tags: Tags as single string.
     :param constraints: constraints as single, comma separated string.
-    :param links_string: Links as single string.
     :param external_css: CSS class name as string, which is set for the <a> tag.
 
     """
@@ -667,7 +672,6 @@ def add_external_need(
         status=status,
         tags=tags,
         constraints=constraints,
-        links_string=links_string,
         is_external=True,
         external_url=external_url,
         external_css=external_css,
diff --git a/sphinx_needs/data.py b/sphinx_needs/data.py
index a97d76c6..b02ba3a5 100644
--- a/sphinx_needs/data.py
+++ b/sphinx_needs/data.py
@@ -6,6 +6,8 @@ from __future__ import annotations
 
 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 @@ if TYPE_CHECKING:
     from sphinx_needs.services.manager import ServiceManager
 
 
+LOGGER = getLogger(__name__)
+
+
 class NeedsFilterType(TypedDict):
     filter: str
     """Filter string."""
@@ -612,7 +617,7 @@ class SphinxNeedsData:
 
 
 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 eaddfe78..3ecc1f3f 100644
--- a/sphinx_needs/external_needs.py
+++ b/sphinx_needs/external_needs.py
@@ -11,7 +11,6 @@ from sphinx.application import Sphinx
 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)
 


Test patch:
diff --git a/tests/doc_test/parallel_doc/index.rst b/tests/doc_test/parallel_doc/index.rst
index 4c318ed9..8b29080b 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 00000000..602d47c9
--- /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 d203541e..1eddf7f6 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 "<h1>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, "<srcdir>/")
+        .strip()
+    )
+    assert (
+        warnings
+        == "<srcdir>/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 "<h1>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 44164ffc..9f2bf042 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 "<h1>PARALLEL TEST DOCUMENT" in html
-    assert "SP_TOO_001" in html
-
+    warnings = (
+        strip_colors(app._warning.getvalue())
+        .replace(str(app.srcdir) + os.path.sep, "<srcdir>/")
+        .strip()
+    )
+    assert (
+        warnings
+        == "<srcdir>/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 "<h1>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