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

Improve Need node creation and content parsing #1168

Merged
merged 14 commits into from
May 8, 2024

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Apr 25, 2024

This PR improves the creation of need nodes and the api.need.add_need() function:

  • It now accepts the content argument as a docutils.StringList, which can be passed directly from a directive.
  • It now accepts a new 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 the needs.json)
  • These arguments are used to improve the reporting of warnings, at the correct line number.
  • The pre_template, if given, is now parsed before the main content

Note, 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.

.. req: title
   :id: my_id

   This is the content, which can have **nested syntax** that we want to parse

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.

.. req: title
   :id: my_id
   :jinja_content: true

   {% for i in range(11) %}
   This is the content.
   After it is "pre-rendered" by jinja, then the content might not align with the original source.
   {% endfor %}

or because it is an external need, loaded from a JSON file, e.g.:

needs_external_needs = [
      {
        'base_url': 'http://mydocs/my_project',
        'json_url':  'http://mydocs/my_project/needs.json',
        'version': '1.0',
        'id_prefix': 'ext_',
        'css_class': 'external_link',
      },
 ]

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:

sourcemap = StringList(content.splitlines(), items=[(source, lineno) for _ in content.splitlines()])
state.nested_parse(sourcemap, ...)

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 in sourcemap, to get the correct place to report about, it actually uses abs_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.

@chrisjsewell chrisjsewell marked this pull request as draft April 25, 2024 10:22
@PhilipPartsch
Copy link
Contributor

PhilipPartsch commented Apr 25, 2024

Cool.
Hint: needs_external_needs are not an issue to been parsed, as external needs are been skipped before parsing. See current api/need.py.

I believe we have to have a look about needimport, especially as they are from an external source but not marked as is_external.

@PhilipPartsch
Copy link
Contributor

I like to see you even thought about pre_content and post_content. I believe we should think to add here the file and lineno from the template file instead of the needs element. Most properly the issue is in the template compared to the need. But how could we inform the user, that the issue is with a dedicated need combined with a template?

Overall I'm not sure if the templates are correctly parsed:

I would first parse the pre_content, than the content and last the post_content. What do you think?
see

Should we skip the pre_content, content and post_content preparation in case of needs coming in with needs_import?
see
Reason: all the templating was done in sphinx-needs build, we fetching the needs.json from.
OR should we rerun the templates, as we might have changed the data filled in with templates e.g. by calling need_extend.
I believe we should at least document the expected behavior. Extra Issue needed?

@danwos danwos added this to the 2024.05 milestone May 2, 2024
@chrisjsewell chrisjsewell marked this pull request as ready for review May 7, 2024 08:39
@chrisjsewell chrisjsewell requested a review from danwos May 7, 2024 08:41
Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 98.30508% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 86.41%. Comparing base (90bae4a) to head (56c72b3).

Files Patch % Lines
sphinx_needs/api/need.py 98.24% 1 Missing ⚠️
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     
Flag Coverage Δ
pytests 86.41% <98.30%> (+0.49%) ⬆️

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.

@chrisjsewell chrisjsewell changed the title Improve nested parsing of needs content, with correct source mapping Improve Need node creation and content parsing May 8, 2024
@chrisjsewell chrisjsewell merged commit 0838672 into master May 8, 2024
19 checks passed
@chrisjsewell chrisjsewell deleted the fix-line-reporting-needs-directive branch May 8, 2024 09:55
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.

None yet

3 participants