Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛👌 Centralise need missing link reporting #1104

Merged
merged 2 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@
# Absolute path to the needs_report_template_file based on the conf.py directory
# needs_report_template = "/needs_templates/report_template.need" # Use custom report template

needs_report_dead_links = False
suppress_warnings = ["needs.link_outgoing"]

needs_types = [
# Architecture types
Expand Down
8 changes: 7 additions & 1 deletion docs/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,13 @@ In this cases, you can provide a list of tuples.
needs_report_dead_links
~~~~~~~~~~~~~~~~~~~~~~~

Deactivate/activate log messages of outgoing dead links. If set to ``False``, then deactivate.
.. deprecated:: 2.1.0

Instead add ``needs.link_outgoing`` to the `suppress_warnings <https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-suppress_warnings>`__ list::

suppress_warnings = ["needs.link_outgoing"]

Deactivate/activate log messages of disallowed outgoing dead links. If set to ``False``, then deactivate.

Default value is ``True``.

Expand Down
1 change: 1 addition & 0 deletions sphinx_needs/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ def __setattr__(self, name: str, value: Any) -> None:
Example: [{"name": "blocks, "incoming": "is blocked by", "copy_link": True, "color": "#ffcc00"}]
"""
report_dead_links: bool = field(default=True, metadata={"rebuild": "html", "types": (bool,)})
"""DEPRECATED: Use ``suppress_warnings = ["needs.link_outgoing"]`` instead."""
filter_data: dict[str, Any] = field(default_factory=dict, metadata={"rebuild": "html", "types": ()})
allow_unsafe_filters: bool = field(default=False, metadata={"rebuild": "html", "types": (bool,)})
flow_show_links: bool = field(default=False, metadata={"rebuild": "html", "types": (bool,)})
Expand Down
29 changes: 23 additions & 6 deletions sphinx_needs/directives/need.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
from sphinx_needs.nodes import Need
from sphinx_needs.utils import add_doc, profile, remove_node_from_tree, split_need_id

logger = get_logger(__name__)
LOGGER = get_logger(__name__)

NON_BREAKING_SPACE = re.compile("\xa0+")

Expand Down Expand Up @@ -156,7 +156,7 @@
if links_string:
for link in re.split(r";|,", links_string):
if link.isspace():
logger.warning(
LOGGER.warning(

Check warning on line 159 in sphinx_needs/directives/need.py

View check run for this annotation

Codecov / codecov/patch

sphinx_needs/directives/need.py#L159

Added line #L159 was not covered by tests
f"Grubby link definition found in need '{self.trimmed_title}'. "
"Defined link contains spaces only. [needs]",
type="needs",
Expand Down Expand Up @@ -436,10 +436,10 @@
the ``has_forbidden_dead_links`` field is also added.
"""
extra_links = config.extra_links
report_dead_links = config.report_dead_links
for need in needs.values():
for link_type in extra_links:
dead_links_allowed = link_type.get("allow_dead_links", False)
need_link_value: List[str] = (
need_link_value = (
[need[link_type["option"]]] if isinstance(need[link_type["option"]], str) else need[link_type["option"]] # type: ignore
)
for need_id_full in need_link_value:
Expand All @@ -449,9 +449,26 @@
need_id_main in needs and need_id_part and need_id_part not in needs[need_id_main]["parts"]
):
need["has_dead_links"] = True
if not dead_links_allowed:
if not link_type.get("allow_dead_links", False):
need["has_forbidden_dead_links"] = True
break # One found dead link is enough
if report_dead_links:
message = f"Need '{need['id']}' has unknown outgoing link '{need_id_full}' in field '{link_type['option']}'"
# if the need has been imported from an external URL,
# we want to provide that URL as the location of the warning,
# otherwise we use the location of the need in the source file
if need.get("is_external", False):
LOGGER.warning(
f"{need['external_url']}: {message} [needs.external_link_outgoing]",
type="needs",
subtype="external_link_outgoing",
)
else:
LOGGER.warning(
f"{message} [needs.link_outgoing]",
location=(need["docname"], need["lineno"]),
type="needs",
subtype="link_outgoing",
)


def create_back_links(needs: Dict[str, NeedsInfoType], config: NeedsSphinxConfig) -> None:
Expand Down
44 changes: 24 additions & 20 deletions sphinx_needs/needs.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,12 @@
NeedFunc: process_need_func,
}

LOGGER = get_logger(__name__)


def setup(app: Sphinx) -> Dict[str, Any]:
log = get_logger(__name__)
log.debug("Starting setup of Sphinx-Needs")
log.debug("Load Sphinx-Data-Viewer for Sphinx-Needs")
LOGGER.debug("Starting setup of Sphinx-Needs")
LOGGER.debug("Load Sphinx-Data-Viewer for Sphinx-Needs")
app.setup_extension("sphinx_data_viewer")
app.setup_extension("sphinxcontrib.jquery")

Expand Down Expand Up @@ -304,20 +305,20 @@
"""
Register extra options and directive based on config from conf.py
"""
log = get_logger(__name__)

needs_config = NeedsSphinxConfig(app.config)

if isinstance(needs_config.extra_options, dict):
log.info(
LOGGER.info(
'Config option "needs_extra_options" supports list and dict. However new default type since '
"Sphinx-Needs 0.7.2 is list. Please see docs for details."
)

extra_options = NEEDS_CONFIG.extra_options
for option in needs_config.extra_options:
if option in extra_options:
log.warning(f'extra_option "{option}" already registered. [needs.config]', type="needs", subtype="config")
LOGGER.warning(
f'extra_option "{option}" already registered. [needs.config]', type="needs", subtype="config"
)
NEEDS_CONFIG.extra_options[option] = directives.unchanged

# Get extra links and create a dictionary of needed options.
Expand Down Expand Up @@ -393,17 +394,24 @@
if name not in NEEDS_CONFIG.warnings:
NEEDS_CONFIG.warnings[name] = check
else:
log.warning(
LOGGER.warning(
f"{name!r} in 'needs_warnings' is already registered. [needs.config]", type="needs", subtype="config"
)

if needs_config.constraints_failed_color:
log.warning(
LOGGER.warning(

Check warning on line 402 in sphinx_needs/needs.py

View check run for this annotation

Codecov / codecov/patch

sphinx_needs/needs.py#L402

Added line #L402 was not covered by tests
'Config option "needs_constraints_failed_color" is deprecated. Please use "needs_constraint_failed_options" styles instead. [needs.config]',
type="needs",
subtype="config",
)

if needs_config.report_dead_links is not True:
LOGGER.warning(

Check warning on line 409 in sphinx_needs/needs.py

View check run for this annotation

Codecov / codecov/patch

sphinx_needs/needs.py#L409

Added line #L409 was not covered by tests
'Config option "needs_constraints_failed_color" is deprecated. Please use `suppress_warnings = ["needs.link_outgoing"]` instead. [needs.config]',
type="needs",
subtype="config",
)


def visitor_dummy(*_args: Any, **_kwargs: Any) -> None:
"""
Expand Down Expand Up @@ -497,19 +505,15 @@


def check_configuration(_app: Sphinx, config: Config) -> None:
"""
Checks the configuration for invalid options.
"""Checks the configuration for invalid options.

E.g. defined need-option, which is already defined internally

:param app:
:param config:
:return:
"""
extra_options = config["needs_extra_options"]
link_types = [x["option"] for x in config["needs_extra_links"]]
needs_config = NeedsSphinxConfig(config)
extra_options = needs_config.extra_options
link_types = [x["option"] for x in needs_config.extra_links]

external_filter = getattr(config, "needs_filter_data", {})
external_filter = needs_config.filter_data
for extern_filter, value in external_filter.items():
# Check if external filter values is really a string
if not isinstance(value, str):
Expand Down Expand Up @@ -545,8 +549,8 @@
" This is not allowed.".format(link + "_back")
)

external_variants = getattr(config, "needs_variants", {})
external_variant_options = getattr(config, "needs_variant_options", [])
external_variants = needs_config.variants
external_variant_options = needs_config.variant_options
for value in external_variants.values():
# Check if external filter values is really a string
if not isinstance(value, str):
Expand Down
39 changes: 6 additions & 33 deletions sphinx_needs/roles/need_outgoing.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ def process_need_outgoing(
builder = app.builder
env = app.env
needs_config = NeedsSphinxConfig(app.config)
report_dead_links = needs_config.report_dead_links
link_lookup = {link["option"]: link for link in needs_config.extra_links}

# for node_need_ref in doctree.findall(NeedOutgoing):
for node_need_ref in found_nodes:
node_link_container = nodes.inline()
Expand Down Expand Up @@ -107,39 +108,11 @@ def process_need_outgoing(
dead_link_para.append(dead_link_text)
node_link_container += dead_link_para

extra_links = getattr(env.config, "needs_extra_links", [])
extra_links_dict = {x["option"]: x for x in extra_links}

# Reduce log level to INFO, if dead links are allowed
if (
"allow_dead_links" in extra_links_dict[link_type]
and extra_links_dict[link_type]["allow_dead_links"]
):
log_level = "INFO"
kwargs = {}
else:
# Set an extra css class, if link type is not configured to allow dead links
# add a CSS class for disallowed unknown links
# note a warning is already emitted when validating the needs list
# so we don't need to do it here
if not link_lookup.get(link_type, {}).get("allow_dead_links", False):
dead_link_para.attributes["classes"].append("forbidden")
log_level = "WARNING"
kwargs = {"type": "needs"}

if report_dead_links:
if node_need_ref and node_need_ref.line:
log.log(
log_level,
f"linked need {need_id_main} not found "
f"(Line {node_need_ref.line} of file {node_need_ref.source}) [needs]",
**kwargs,
)
else:
log.log(
log_level,
"outgoing linked need {} not found (document: {}, "
"source need {} on line {} ) [needs]".format(
need_id_main, ref_need["docname"], ref_need["id"], ref_need["lineno"]
),
**kwargs,
)

# If we have several links, we add an empty text between them
if (index + 1) < len(link_list):
Expand Down
3 changes: 2 additions & 1 deletion sphinx_needs/roles/need_ref.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,9 @@ def process_need_ref(app: Sphinx, doctree: nodes.document, fromdocname: str, fou

else:
log.warning(
f"linked need {node_need_ref['reftarget']} not found [needs]",
f"linked need {node_need_ref['reftarget']} not found [needs.link_ref]",
type="needs",
subtype="link_ref",
location=node_need_ref,
)

Expand Down
14 changes: 9 additions & 5 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from sphinx import version_info
from sphinx.application import Sphinx
from sphinx.testing.path import path
from sphinx.testing.util import SphinxTestApp
from syrupy.extensions.single_file import SingleFileSnapshotExtension, WriteMode
from xprocess import ProcessStarter

Expand Down Expand Up @@ -231,10 +232,11 @@ def test_app(make_app, sphinx_test_tempdir, request):
builder_params = request.param

sphinx_conf_overrides = builder_params.get("confoverrides", {})
# Since we don't want copy the plantuml.jar file for each test function,
# we need to override the plantuml conf variable and set it to what we have already
plantuml = "java -Djava.awt.headless=true -jar %s" % os.path.join(sphinx_test_tempdir, "utils", "plantuml.jar")
sphinx_conf_overrides.update(plantuml=plantuml)
if not builder_params.get("no_plantuml", False):
# Since we don't want copy the plantuml.jar file for each test function,
# we need to override the plantuml conf variable and set it to what we have already
plantuml = "java -Djava.awt.headless=true -jar %s" % os.path.join(sphinx_test_tempdir, "utils", "plantuml.jar")
sphinx_conf_overrides.update(plantuml=plantuml)

# copy test srcdir to test temporary directory sphinx_test_tempdir
srcdir = builder_params.get("srcdir")
Expand All @@ -245,7 +247,7 @@ def test_app(make_app, sphinx_test_tempdir, request):
src_dir = Path(str(src_dir))

# return sphinx.testing fixture make_app and new srcdir which is in sphinx_test_tempdir
app: Sphinx = make_app(
app: SphinxTestApp = make_app(
buildername=builder_params.get("buildername", "html"),
srcdir=src_dir,
freshenv=builder_params.get("freshenv"),
Expand All @@ -268,6 +270,8 @@ def test_app(make_app, sphinx_test_tempdir, request):

yield app

app.cleanup()

# Clean up the srcdir of each Sphinx app after the test function has executed
if request.config.getoption("--sn-build-dir") is None:
shutil.rmtree(parent_path, ignore_errors=True)
Expand Down
2 changes: 1 addition & 1 deletion tests/doc_test/doc_report_dead_links_false/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
{"directive": "test", "title": "Test Case", "prefix": "TC_", "color": "#DCB239", "style": "node"},
]

needs_report_dead_links = False
suppress_warnings = ["needs.link_outgoing"]

needs_extra_links = [
{
Expand Down
24 changes: 18 additions & 6 deletions tests/test_basic_doc.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@

import pytest
import responses
from sphinx import version_info
from sphinx.application import Sphinx
from sphinx.testing.util import SphinxTestApp
from syrupy.filters import props

from sphinx_needs.api.need import NeedsNoIdException
Expand Down Expand Up @@ -234,14 +236,24 @@ def test_sphinx_api_build():
temp_dir = tempfile.mkdtemp()
src_dir = os.path.join(os.path.dirname(__file__), "doc_test", "doc_basic")

sphinx_app = Sphinx(
if version_info >= (7, 2):
src_dir = Path(src_dir)
temp_dir = Path(temp_dir)
else:
from sphinx.testing.path import path

src_dir = path(src_dir)
temp_dir = path(temp_dir)

sphinx_app = SphinxTestApp(
srcdir=src_dir,
confdir=src_dir,
outdir=temp_dir,
doctreedir=temp_dir,
builddir=temp_dir,
buildername="html",
parallel=4,
freshenv=True,
)
sphinx_app.build()
assert sphinx_app.statuscode == 0
try:
sphinx_app.build()
assert sphinx_app.statuscode == 0
finally:
sphinx_app.cleanup()
17 changes: 11 additions & 6 deletions tests/test_broken_links.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
import pytest
from sphinx.util.console import strip_colors


@pytest.mark.parametrize("test_app", [{"buildername": "html", "srcdir": "doc_test/broken_links"}], indirect=True)
@pytest.mark.parametrize(
"test_app", [{"buildername": "html", "srcdir": "doc_test/broken_links", "no_plantuml": True}], indirect=True
)
def test_doc_build_html(test_app):
app = test_app
app.build()

warning = app._warning
# stdout warnings
warnings = warning.getvalue()

assert "linked need BROKEN_LINK not found" in warnings
# check there are expected warnings
warnings = strip_colors(app._warning.getvalue().replace(str(app.srcdir), "srcdir"))
print(warnings.splitlines())
assert warnings.splitlines() == [
"srcdir/index.rst:12: WARNING: Need 'SP_TOO_002' has unknown outgoing link 'NOT_WORKING_LINK' in field 'links' [needs.link_outgoing]",
"srcdir/index.rst:21: WARNING: linked need BROKEN_LINK not found [needs.link_ref]",
]
Loading
Loading