From 1126c90f2019dc6f7401082e3c7feb0c2cd41004 Mon Sep 17 00:00:00 2001
From: Chris Sewell <chrisj_sewell@hotmail.com>
Date: Fri, 23 Feb 2024 00:25:11 +0100
Subject: [PATCH] =?UTF-8?q?=F0=9F=91=8C=20Improve=20warnings=20for=20bad?=
 =?UTF-8?q?=20filters=20(add=20source=20location=20and=20subtype)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 sphinx_needs/api/need.py                      | 16 +++++++---
 sphinx_needs/builder.py                       | 12 +++++--
 sphinx_needs/directives/needbar.py            |  9 ++++--
 sphinx_needs/directives/needextend.py         | 15 ++++-----
 sphinx_needs/directives/needgantt.py          | 14 ++++++--
 sphinx_needs/directives/needpie.py            |  9 ++++--
 sphinx_needs/filter_common.py                 | 32 +++++++++++++------
 sphinx_needs/functions/common.py              |  6 +++-
 sphinx_needs/roles/need_count.py              | 23 +++++++++++--
 sphinx_needs/warnings.py                      |  5 ++-
 tests/doc_test/filter_doc/filter_all.rst      | 15 +++++----
 tests/doc_test/filter_doc/filter_no_needs.rst |  5 +--
 tests/doc_test/filter_doc/filter_search.rst   | 14 ++++----
 tests/doc_test/filter_doc/filter_tags_or.rst  |  8 +++--
 tests/doc_test/filter_doc/index.rst           | 25 +++++++++++----
 tests/test_filter.py                          | 13 ++++++++
 16 files changed, 161 insertions(+), 60 deletions(-)

diff --git a/sphinx_needs/api/need.py b/sphinx_needs/api/need.py
index f376257eb..ec5c39f7a 100644
--- a/sphinx_needs/api/need.py
+++ b/sphinx_needs/api/need.py
@@ -153,8 +153,10 @@ def run():
     if need_type not in configured_need_types:
         logger.warning(
             f"Couldn't create need {id}. Reason: The need-type (i.e. `{need_type}`) is not set "
-            "in the project's 'need_types' configuration in conf.py. [needs]",
+            "in the project's 'need_types' configuration in conf.py. [needs.add]",
             type="needs",
+            subtype="add",
+            location=(docname, lineno) if docname else None,
         )
 
     for ntype in types:
@@ -219,9 +221,11 @@ def run():
         for i in range(len(tags)):
             if len(tags[i]) == 0 or tags[i].isspace():
                 logger.warning(
-                    f"Scruffy tag definition found in need {need_id}. "
-                    "Defined tag contains spaces only. [needs]",
+                    f"Scruffy tag definition found in need {need_id!r}. "
+                    "Defined tag contains spaces only. [needs.add]",
                     type="needs",
+                    subtype="add",
+                    location=(docname, lineno) if docname else None,
                 )
             else:
                 new_tags.append(tags[i])
@@ -256,9 +260,11 @@ def run():
         for i in range(len(constraints)):
             if len(constraints[i]) == 0 or constraints[i].isspace():
                 logger.warning(
-                    f"Scruffy tag definition found in need {need_id}. "
-                    "Defined constraint contains spaces only. [needs]",
+                    f"Scruffy constraint definition found in need {need_id!r}. "
+                    "Defined constraint contains spaces only. [needs.add]",
                     type="needs",
+                    subtype="add",
+                    location=(docname, lineno) if docname else None,
                 )
             else:
                 new_constraints.append(constraints[i])
diff --git a/sphinx_needs/builder.py b/sphinx_needs/builder.py
index b4ebea207..21114c091 100644
--- a/sphinx_needs/builder.py
+++ b/sphinx_needs/builder.py
@@ -85,7 +85,10 @@ def finish(self) -> None:
 
         filter_string = needs_config.builder_filter
         filtered_needs: list[NeedsInfoType] = filter_needs(
-            data.get_or_create_needs().values(), needs_config, filter_string
+            data.get_or_create_needs().values(),
+            needs_config,
+            filter_string,
+            append_warning="(from need_builder_filter)",
         )
 
         for need in filtered_needs:
@@ -182,7 +185,12 @@ def finish(self) -> None:
         filter_string = needs_config.builder_filter
         from sphinx_needs.filter_common import filter_needs
 
-        filtered_needs = filter_needs(needs, needs_config, filter_string)
+        filtered_needs = filter_needs(
+            needs,
+            needs_config,
+            filter_string,
+            append_warning="(from need_builder_filter)",
+        )
         needs_build_json_per_id_path = needs_config.build_json_per_id_path
         needs_dir = os.path.join(self.outdir, needs_build_json_per_id_path)
         if not os.path.exists(needs_dir):
diff --git a/sphinx_needs/directives/needbar.py b/sphinx_needs/directives/needbar.py
index 598b425a8..f41c286f2 100644
--- a/sphinx_needs/directives/needbar.py
+++ b/sphinx_needs/directives/needbar.py
@@ -157,7 +157,10 @@ def run(self) -> Sequence[nodes.Node]:
 
         add_doc(env, env.docname)
 
-        return [targetnode, Needbar("")]
+        bar_node = Needbar("")
+        self.set_source_info(bar_node)
+
+        return [targetnode, bar_node]
 
 
 # Algorithm:
@@ -301,7 +304,9 @@ def process_needbar(
                 if element.isdigit():
                     line_number.append(float(element))
                 else:
-                    result = len(filter_needs(need_list, needs_config, element))
+                    result = len(
+                        filter_needs(need_list, needs_config, element, location=node)
+                    )
                     line_number.append(float(result))
             local_data_number.append(line_number)
 
diff --git a/sphinx_needs/directives/needextend.py b/sphinx_needs/directives/needextend.py
index 054d97f53..b123da3c6 100644
--- a/sphinx_needs/directives/needextend.py
+++ b/sphinx_needs/directives/needextend.py
@@ -98,15 +98,12 @@ def extend_needs_data(
                 logger.info(error)
                 continue
         else:
-            # a filter string
-            try:
-                found_needs = filter_needs(
-                    all_needs.values(), needs_config, need_filter
-                )
-            except NeedsInvalidFilter as e:
-                raise NeedsInvalidFilter(
-                    f"Filter not valid for needextend on page {current_needextend['docname']}:\n{e}"
-                )
+            found_needs = filter_needs(
+                all_needs.values(),
+                needs_config,
+                need_filter,
+                location=(current_needextend["docname"], current_needextend["lineno"]),
+            )
 
         for found_need in found_needs:
             # Work in the stored needs, not on the search result
diff --git a/sphinx_needs/directives/needgantt.py b/sphinx_needs/directives/needgantt.py
index 5f8e8f3f1..aed1a52aa 100644
--- a/sphinx_needs/directives/needgantt.py
+++ b/sphinx_needs/directives/needgantt.py
@@ -124,7 +124,10 @@ def run(self) -> Sequence[nodes.Node]:
 
         add_doc(env, env.docname)
 
-        return [targetnode] + [Needgantt("")]
+        gantt_node = Needgantt("")
+        self.set_source_info(gantt_node)
+
+        return [targetnode, gantt_node]
 
     def get_link_type_option(self, name: str, default: str = "") -> list[str]:
         link_types = [
@@ -244,10 +247,17 @@ def process_needgantt(
                 complete_option = current_needgantt["completion_option"]
                 complete = need[complete_option]  # type: ignore[literal-required]
                 if not (duration and duration.isdigit()):
+                    need_location = (
+                        f" (located: {need['docname']}:{need['lineno']})"
+                        if need["docname"]
+                        else ""
+                    )
                     logger.warning(
                         "Duration not set or invalid for needgantt chart. "
-                        "Need: {}. Duration: {} [needs]".format(need["id"], duration),
+                        f"Need: {need['id']!r}{need_location}. Duration: {duration!r} [needs.gantt]",
                         type="needs",
+                        subtype="gantt",
+                        location=node,
                     )
                     duration = 1
                 gantt_element = "[{}] as [{}] lasts {} days\n".format(
diff --git a/sphinx_needs/directives/needpie.py b/sphinx_needs/directives/needpie.py
index 700c25247..a01eb032e 100644
--- a/sphinx_needs/directives/needpie.py
+++ b/sphinx_needs/directives/needpie.py
@@ -102,7 +102,10 @@ def run(self) -> Sequence[nodes.Node]:
         }
         add_doc(env, env.docname)
 
-        return [targetnode, Needpie("")]
+        pie_node = Needpie("")
+        self.set_source_info(pie_node)
+
+        return [targetnode, pie_node]
 
 
 @measure_time("needpie")
@@ -162,7 +165,9 @@ def process_needpie(
                 if line.isdigit():
                     sizes.append(abs(float(line)))
                 else:
-                    result = len(filter_needs(need_list, needs_config, line))
+                    result = len(
+                        filter_needs(need_list, needs_config, line, location=node)
+                    )
                     sizes.append(result)
         elif current_needpie["filter_func"] and not content:
             try:
diff --git a/sphinx_needs/filter_common.py b/sphinx_needs/filter_common.py
index 0ba740372..be4c02c99 100644
--- a/sphinx_needs/filter_common.py
+++ b/sphinx_needs/filter_common.py
@@ -9,6 +9,7 @@
 from types import CodeType
 from typing import Any, Iterable, TypedDict, TypeVar
 
+from docutils import nodes
 from docutils.parsers.rst import directives
 from sphinx.application import Sphinx
 from sphinx.util.docutils import SphinxDirective
@@ -180,7 +181,10 @@ def process_filters(
                     found_needs_by_options.append(need_info)
             # Get need by filter string
             found_needs_by_string = filter_needs(
-                all_needs_incl_parts, needs_config, filter_data["filter"]
+                all_needs_incl_parts,
+                needs_config,
+                filter_data["filter"],
+                location=(filter_data["docname"], filter_data["lineno"]),
             )
             # Make an intersection of both lists
             found_needs = intersection_of_need_results(
@@ -190,7 +194,10 @@ def process_filters(
             # There is no other config as the one for filter string.
             # So we only need this result.
             found_needs = filter_needs(
-                all_needs_incl_parts, needs_config, filter_data["filter"]
+                all_needs_incl_parts,
+                needs_config,
+                filter_data["filter"],
+                location=(filter_data["docname"], filter_data["lineno"]),
             )
     else:
         # Provides only a copy of needs to avoid data manipulations.
@@ -296,6 +303,9 @@ def filter_needs(
     config: NeedsSphinxConfig,
     filter_string: None | str = "",
     current_need: NeedsInfoType | None = None,
+    *,
+    location: tuple[str, int | None] | nodes.Node | None = None,
+    append_warning: str = "",
 ) -> list[V]:
     """
     Filters given needs based on a given filter string.
@@ -305,6 +315,8 @@ def filter_needs(
     :param config: NeedsSphinxConfig object
     :param filter_string: strings, which gets evaluated against each need
     :param current_need: current need, which uses the filter.
+    :param location: source location for error reporting (docname, line number)
+    :param append_warning: additional text to append to any failed filter warning
 
     :return: list of found needs
     """
@@ -328,13 +340,15 @@ def filter_needs(
             ):
                 found_needs.append(filter_need)
         except Exception as e:
-            if not error_reported:  # Let's report a filter-problem only onces
-                location = (
-                    (current_need["docname"], current_need["lineno"])
-                    if current_need
-                    else None
+            if not error_reported:  # Let's report a filter-problem only once
+                if append_warning:
+                    append_warning = f" {append_warning}"
+                log.warning(
+                    f"{e}{append_warning} [needs.filter]",
+                    type="needs",
+                    subtype="filter",
+                    location=location,
                 )
-                log.warning(str(e) + " [needs]", type="needs", location=location)
                 error_reported = True
 
     return found_needs
@@ -381,5 +395,5 @@ def filter_single_need(
         else:
             result = bool(eval(filter_string, filter_context))
     except Exception as e:
-        raise NeedsInvalidFilter(f"Filter {filter_string} not valid. Error: {e}.")
+        raise NeedsInvalidFilter(f"Filter {filter_string!r} not valid. Error: {e}.")
     return result
diff --git a/sphinx_needs/functions/common.py b/sphinx_needs/functions/common.py
index 467743294..4ab57647f 100644
--- a/sphinx_needs/functions/common.py
+++ b/sphinx_needs/functions/common.py
@@ -166,7 +166,11 @@ def copy(
 
     if filter:
         result = filter_needs(
-            needs.values(), NeedsSphinxConfig(app.config), filter, need
+            needs.values(),
+            NeedsSphinxConfig(app.config),
+            filter,
+            need,
+            location=(need["docname"], need["lineno"]),
         )
         if result:
             need = result[0]
diff --git a/sphinx_needs/roles/need_count.py b/sphinx_needs/roles/need_count.py
index 15275a090..0d6577181 100644
--- a/sphinx_needs/roles/need_count.py
+++ b/sphinx_needs/roles/need_count.py
@@ -37,11 +37,28 @@ def process_need_count(
             filters = filter.split(" ? ")
             if len(filters) == 1:
                 need_list = prepare_need_list(all_needs)  # adds parts to need_list
-                amount = str(len(filter_needs(need_list, needs_config, filters[0])))
+                amount = str(
+                    len(
+                        filter_needs(
+                            need_list,
+                            needs_config,
+                            filters[0],
+                            location=node_need_count,
+                        )
+                    )
+                )
             elif len(filters) == 2:
                 need_list = prepare_need_list(all_needs)  # adds parts to need_list
-                amount_1 = len(filter_needs(need_list, needs_config, filters[0]))
-                amount_2 = len(filter_needs(need_list, needs_config, filters[1]))
+                amount_1 = len(
+                    filter_needs(
+                        need_list, needs_config, filters[0], location=node_need_count
+                    )
+                )
+                amount_2 = len(
+                    filter_needs(
+                        need_list, needs_config, filters[1], location=node_need_count
+                    )
+                )
                 amount = f"{amount_1 / amount_2 * 100:2.1f}"
             elif len(filters) > 2:
                 raise NeedsInvalidFilter(
diff --git a/sphinx_needs/warnings.py b/sphinx_needs/warnings.py
index 1f9a47c2c..658c67dfc 100644
--- a/sphinx_needs/warnings.py
+++ b/sphinx_needs/warnings.py
@@ -62,7 +62,10 @@ def process_warnings(app: Sphinx, exception: Exception | None) -> None:
             if isinstance(warning_filter, str):
                 # filter string used
                 result = filter_needs(
-                    checked_needs.values(), needs_config, warning_filter
+                    checked_needs.values(),
+                    needs_config,
+                    warning_filter,
+                    append_warning=f"(from warning filter {warning_name!r})",
                 )
             elif callable(warning_filter):
                 # custom defined filter code used from conf.py
diff --git a/tests/doc_test/filter_doc/filter_all.rst b/tests/doc_test/filter_doc/filter_all.rst
index 4f9ae9d04..1cd55f491 100644
--- a/tests/doc_test/filter_doc/filter_all.rst
+++ b/tests/doc_test/filter_doc/filter_all.rst
@@ -2,30 +2,33 @@ filter_all
 ==========
 
 .. req:: req_a_not
-   :tags: 1;
+   :tags: 1
    :status: open
    :hide:
+   :duration: 1
 
 .. req:: req_b_found
-   :tags: 2;
+   :tags: 2
    :status: closed
 
 .. req:: req_c_not
-   :tags: 1;2;
+   :tags: 1;2
    :status: open
    :hide:
+   :duration: 1
 
 .. req:: req_d_found
-   :tags: 1;2;
+   :tags: 1;2
    :status: closed
+   :duration: 1
 
 .. story:: story_1_not
-   :tags: 3;
+   :tags: 3
    :status: none
    :hide:
 
 .. story:: story_2_found
-   :tags: 2;
+   :tags: 2
    :status: none
 
 .. test:: my_test
diff --git a/tests/doc_test/filter_doc/filter_no_needs.rst b/tests/doc_test/filter_doc/filter_no_needs.rst
index 8cb606e82..40c7c006d 100644
--- a/tests/doc_test/filter_doc/filter_no_needs.rst
+++ b/tests/doc_test/filter_doc/filter_no_needs.rst
@@ -3,13 +3,14 @@ filter_no_needs
 
 .. req:: filter_warning_req_a
    :id: FILTER_001
-   :tags: 1;
+   :tags: 1
    :status: open
    :hide:
+   :duration: 1
 
 .. req:: filter_warning_req_b
    :id: FILTER_002
-   :tags: 2;
+   :tags: 2
    :status: closed
    :hide:
 
diff --git a/tests/doc_test/filter_doc/filter_search.rst b/tests/doc_test/filter_doc/filter_search.rst
index dfb0c57d9..ce6613f85 100644
--- a/tests/doc_test/filter_doc/filter_search.rst
+++ b/tests/doc_test/filter_doc/filter_search.rst
@@ -3,40 +3,40 @@ filter_search
 
 .. req:: search_a
    :id: AAAA
-   :tags: filter_search;
+   :tags: filter_search
    :status: open
 
 .. req:: search_b
    :id: BBB
-   :tags: filter_search;
+   :tags: filter_search
    :status: closed
    :hide:
 
 .. req:: search_c
    :id: CCC
-   :tags: filter_search;
+   :tags: filter_search
    :status: open
    :hide:
 
 .. req:: search_d
    :id: DDD
-   :tags: filter_search;
+   :tags: filter_search
    :status: closed
    :hide:
 
 .. story:: search_2_1
    :id: ABC
-   :tags: filter_search;
+   :tags: filter_search
    :status: none
 
 .. story:: search_2_2
    :id: CBA
-   :tags: filter_search;
+   :tags: filter_search
    :status: none
 
 .. test:: test_email
    :id: TEST_123
-   :tags: filter_search;
+   :tags: filter_search
    :status: none
 
    me@myemail.com
diff --git a/tests/doc_test/filter_doc/filter_tags_or.rst b/tests/doc_test/filter_doc/filter_tags_or.rst
index f7cc23b0f..85a21c455 100644
--- a/tests/doc_test/filter_doc/filter_tags_or.rst
+++ b/tests/doc_test/filter_doc/filter_tags_or.rst
@@ -5,13 +5,15 @@ Testing filter with and or
 --------------------------
 
 .. req:: req_a
-   :tags: 1;
+   :tags: 1
+   :duration: 1
 
 .. req:: req_b
-   :tags: 2;
+   :tags: 2
 
 .. req:: req_c
-   :tags: 1;2;
+   :tags: 1;2
+   :duration: 1
 
 .. needfilter::
    :filter: "1" in tags or "2" in tags
diff --git a/tests/doc_test/filter_doc/index.rst b/tests/doc_test/filter_doc/index.rst
index ce43a9ff3..d5ab0643b 100644
--- a/tests/doc_test/filter_doc/index.rst
+++ b/tests/doc_test/filter_doc/index.rst
@@ -13,14 +13,14 @@ Testing simple filter
 ---------------------
 
 .. story:: story_a_1
-   :tags: a;
+   :tags: a
 
 .. story:: story_b_1
-   :tags: b;
+   :tags: b
    :hide:
 
 .. story:: story_a_b_1
-   :tags: a;b;
+   :tags: a;b
 
 .. needfilter::
    :filter: "a" in tags
@@ -30,15 +30,28 @@ Testing filter with and or
 --------------------------
 
 .. req:: req_a_1
-   :tags: 1;
+   :tags: 1
    :hide:
+   :duration: 1
 
 .. req:: req_b_1
-   :tags: 2;
+   :tags: 2
    :hide:
 
 .. req:: req_c_1
-   :tags: 1;2;
+   :tags: 1;2
+   :duration: 1
 
 .. needfilter::
    :filter: "1" in tags and "2" in tags
+
+Testing bad filters
+-------------------
+
+.. needfilter::
+   :filter: xxx
+
+.. needlist::
+   :filter: yyy
+
+:need_count:`zzz`
diff --git a/tests/test_filter.py b/tests/test_filter.py
index f8c44b00d..a1834d601 100644
--- a/tests/test_filter.py
+++ b/tests/test_filter.py
@@ -1,6 +1,7 @@
 from pathlib import Path
 
 import pytest
+from sphinx.util.console import strip_colors
 
 
 @pytest.mark.parametrize(
@@ -11,6 +12,18 @@
 def test_filter_build_html(test_app):
     app = test_app
     app.build()
+
+    warnings = strip_colors(
+        app._warning.getvalue().replace(str(app.srcdir), "srcdir")
+    ).splitlines()
+    for w in warnings:
+        print(w)
+    assert warnings == [
+        "srcdir/index.rst:51: WARNING: Filter 'xxx' not valid. Error: name 'xxx' is not defined. [needs.filter]",
+        "srcdir/index.rst:54: WARNING: Filter 'yyy' not valid. Error: name 'yyy' is not defined. [needs.filter]",
+        "srcdir/index.rst:57: WARNING: Filter 'zzz' not valid. Error: name 'zzz' is not defined. [needs.filter]",
+    ]
+
     html = Path(app.outdir, "index.html").read_text()
     assert "story_a_1" in html
     assert "story_b_1" not in html