-
Notifications
You must be signed in to change notification settings - Fork 72
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
Improve Need
node creation and content parsing
#1168
Conversation
for more information, see https://pre-commit.ci
Cool. I believe we have to have a look about needimport, especially as they are from an external source but not marked as |
I like to see you even thought about pre_content and post_content. I believe we should think to add here the Overall I'm not sure if the templates are correctly parsed: I would first parse the Should we skip the |
for more information, see https://pre-commit.ci
…om/useblocks/sphinx-needs into fix-line-reporting-needs-directive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1168 +/- ##
==========================================
+ Coverage 85.92% 86.41% +0.49%
==========================================
Files 56 56
Lines 6536 6531 -5
==========================================
+ Hits 5616 5644 +28
+ Misses 920 887 -33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Need
node creation and content parsing
This PR improves the creation of need nodes and the
api.need.add_need()
function:content
argument as adocutils.StringList
, which can be passed directly from a directive.lineno_content
argument, which denotes the starting line number of the content, within the directive.lineno_content
is stored within the internal need data index (although for now it is not output in theneeds.json
)pre_template
, if given, is now parsed before the main contentNote, it also deals with fixing some issues in the test suite arising from sphinx v7.3
original comment:
This is a WIP based on @PhilipPartsch original PR (#1150), where the main goal is to have sphinx warnings point to a sensible location in the source documentation (file-path and line-number)
The main difference being that here, where possible we simply pass through the
StringList
object (a "source-mapped" representation of the content) from the actual directive and pass that.This works best for needs directives that are specified directly on the page, e.g.
If the content is not directly on the page, then this is where I have found things becoming tricky.
This could either be because, the need contains jinja content, e.g.
or because it is an external need, loaded from a JSON file, e.g.:
For both of these, ideally we want to construct our own source-mapping object to pass to docutils/sphinx: https://github.com/live-clones/docutils/blob/582f8c946f776da7db652c06e0b49521f9dc3fdf/docutils/docutils/statemachine.py#L1323
For example, we might want to simply point every line to the start of the directive:
The appears to me, to be a minor technical annoyance here, in that docutils ignores this source-mapping, when it comes to issue warnings, for example here: https://github.com/live-clones/docutils/blob/582f8c946f776da7db652c06e0b49521f9dc3fdf/docutils/docutils/parsers/rst/states.py#L429
instead of using
StateMachine.get_source_and_line(lineno)
, which returns what we set insourcemap
, to get the correct place to report about, it actually usesabs_line_number
, which just report the line offset in.This is probably a bit technical for people reading this, but I write it here asI think I will eventually upstream the question as a bug to docutils.