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

Conversation

chrisjsewell
Copy link
Member

Currently, warnings for need outgoing links that reference a non-existent ID, are only generated "indirectly" in the render phase; where NeedOutgoing nodes are converted to nodes that are understood by sphinx builders (HTML, PDF, ...).

Since this render phase is no longer needed/run for simply creating the needs.json, using the needs builder, these warnings are not generated.

Additionally, any needs that are not explicitly rendered in the documentation, like externally imported needs, are also skipped.

The reporting of unknown links is now moved to the check_links function, meaning it is now run for all needs and all builders. The warnings have also been given a subtype link_outgoing or external_link_outgoing, e.g.

srcdir/index.rst:12: WARNING: Need 'SP_TOO_002' has unknown outgoing link 'NOT_WORKING_LINK' in field 'links' [needs.link_outgoing]
WARNING: http://my_company.com/docs/v1/index.html#TEST_01: Need 'EXT_TEST_01' has unknown outgoing link 'SPEC_1' in field 'links' [needs.external_link_outgoing]

This means they can be suppressed using the standard Sphinx config:

suppress_warnings = ["needs.link_outgoing", "needs.external_link_outgoing"]` 

This deprecates the need for the needs_report_dead_links configuration, which now emits a deprecation warning if set by the user.

@danwos, do you agree that needs_report_dead_links can be deprecated, and that warnings should be emitted for unknown ids in external need links?

closes #1038
closes #1095

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (f083bbd) 83.84% compared to head (ffc7be0) 83.83%.

Files Patch % Lines
sphinx_needs/needs.py 86.66% 2 Missing ⚠️
sphinx_needs/directives/need.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1104      +/-   ##
==========================================
- Coverage   83.84%   83.83%   -0.01%     
==========================================
  Files          56       56              
  Lines        6424     6421       -3     
==========================================
- Hits         5386     5383       -3     
  Misses       1038     1038              
Flag Coverage Δ
pytests 83.83% <89.28%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@danwos danwos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. Looks good 👍 And I also agree needs_report_dead_links is deprecated with this change-

Currently, warnings for need outgoing links that reference a non-existent ID, are only generated "indirectly" in the render phase; where  `NeedOutgoing` nodes are converted to nodes that are understood by sphinx builders (HTML, PDF, ...).

Since this render phase is no longer needed/run for simply creating the `needs.json`, using the `needs` builder, these warnings are not generated.

Additionally, any needs that are not explicitly rendered in the documentation, like externally imported needs, are also skipped.

The reporting of unknown links is now moved to the `check_links` function, meaning it is now run for all needs and all builders.
The warnings have also been given a subtype `link_outgoing` or `external_link_outgoing`, meaning they can be suppressed using the standard Sphinx config: `suppress_warnings = ["needs.link_outgoing", "needs.external_link_outgoing"]`
This deprecates the need for the `needs_report_dead_links` configuration, which now emits a deprecation warning if set by the user.
@chrisjsewell chrisjsewell merged commit 6b26526 into master Feb 15, 2024
19 checks passed
@chrisjsewell chrisjsewell deleted the fix/1095-warn-links branch February 15, 2024 14:39
@arwedus
Copy link
Contributor

arwedus commented Mar 4, 2024

Hi @danwos and @chrisjsewell , I've noticed in a local test that this does not raise warnings in case I've changed a need ID but forgot to update the links to it in other needs, when I'm building with the needs builder (or inventories builder).

/edit: Nevermind, it pulled in sphinx-needs 2.0.0 instead of master. Works now. Any chance of a new minor release soon?

@chrisjsewell
Copy link
Member Author

Nevermind, it pulled in sphinx-needs 2.0.0 instead of master. Works now. Any chance of a new minor release soon?

Ah good 😄

yes I hope soon, I'd just like to push to try and finalise some of my outstanding PRs first 😅
probably another major, since some of the changes are "technically" a little breaking, but if you are already trying out master, then thats a good sign

@arwedus
Copy link
Contributor

arwedus commented Mar 5, 2024

Nevermind, it pulled in sphinx-needs 2.0.0 instead of master. Works now. Any chance of a new minor release soon?

Ah good 😄

yes I hope soon, I'd just like to push to try and finalise some of my outstanding PRs first 😅 probably another major, since some of the changes are "technically" a little breaking, but if you are already trying out master, then thats a good sign

@chrisjsewell and @danwos: Looking forward to it. Even if it's a new major, please don't drop support for Python 3.8 yet, if any way possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

needs.json builder seems to not check links Add sphinx warning for dead link
3 participants