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

Needs list shall take into account "only" directive #1103

Open
David-Le-Nir opened this issue Feb 8, 2024 · 11 comments · May be fixed by #1112
Open

Needs list shall take into account "only" directive #1103

David-Le-Nir opened this issue Feb 8, 2024 · 11 comments · May be fixed by #1112

Comments

@David-Le-Nir
Copy link
Contributor

David-Le-Nir commented Feb 8, 2024

When using the only directive some needs may require to be excluded from the needs list, i.e. from needs.json, needtable, needpie ...

For example:

.. only:: tag1

  .. req:: requirement for tag1
       :id: req-001

       This one shall only appear when building tag1

.. req:: requirement for all tags
     :id: req-002

       This one shall always appear

in this case when I don't build tag1, I don't want req-001 to appear in needs.json or in needtable

Do you agree about the expected behaviour ?
Do you know if while processing the need level we are able to know that the element is under an "only" block ?

just for information, we found this interesting extension which can be inspiring: https://github.com/pfalcon/sphinx_selective_exclude

@chrisjsewell
Copy link
Member

Heya, oo tricky one

So, setting aside the logic of how to actually use this data for now, I think we may be able to capture the only tags on the needs data item, within the analyse_need_locations function.

Here you see we already capture what parent sections the need is under, so we may be able to do the same thing to capture what parent only the need is under:

def analyse_need_locations(app: Sphinx, doctree: nodes.document) -> None:

@David-Le-Nir
Copy link
Contributor Author

would removing the nodes as it is done for hidden needs be sufficient ?
or it is only about the display ?

@chrisjsewell
Copy link
Member

chrisjsewell commented Feb 9, 2024

would removing the nodes as it is done for hidden needs be sufficient ? or it is only about the display ?

Only about display
I feel #1106 misunderstands hidden needs: they don't remove the need data, merely its represenataion in the output (e.g. in the HTML)

Remove data at this stage is not viable, because it would not work for multiple (cached) builds, using different tags / builders, i.e. subsequent builds would always have this data removed, even if you change the tags

The cached data (from the read phase) needs to be agnostic of builder / tags,
hence why I said you need to store the only expressions in the need data item.
Then, later in the process, during the builder/tag specific write phase, you decide what needs to omit

@David-Le-Nir
Copy link
Contributor Author

Thanks for the explanation.
I started a PR but, I don't know why, I do not manage to run tests.

Anyway, adding a field to the needs is ok. I propose to add boolean field named "excluded_by_only".
I'm not very happy with this name but could not find a better one. Please advise for another name if you have a better one.

I think excluding from the html builder is already handled.
I'll look how to exclude from the needs builder and also for directive like needtable, needpie, needlist.
Maybe there is one place to rule them all ?

@danwos
Copy link
Member

danwos commented Feb 12, 2024

I somehow hate the only directive, as all the content gets rendered every time and gets then removed later by the builder.
That also makes trouble for directives like toctree and others.

For me, the builder, somewhere in Sphinx, or the only directive would be the correct place to solve this. But I guess this will never happen :/

For the above reason, I needed to add an if-builder directive to sphinx-simplepdf. Its content gets not rendered and therefore is working with sphinx-needs and toctree.
https://sphinx-simplepdf.readthedocs.io/en/latest/directives.html#if-builder

I also see a lot of corner cases, if the need-directive is embedded inside other needs (list-tables, container, other needs, ...).
There can be so many, and checking all the node tree upwards may cost some performance.

I'm currently not sure what the best solution is.

@David-Le-Nir
Copy link
Contributor Author

David-Le-Nir commented Feb 12, 2024

Do you mean if the if-builder accepted tags it could greatly replace the only ? (I didn't check the code, sorry)

Maybe adding the boolean field at the need level is an acceptable first step for the moment as it will allow to manually filter in needs.json or needtables and so on. I just have to ensure it is correctly computed for nested containers.

But this is not very user friendly and should be documented so that users do not forget to manually filter this boolean field when they are using the only directive.

edit: another possibility, instead of the boolean would be to copy the tags from the only level to the need, so manually filtering would be more comprehensible for user. Either by appending the the tags field (which content will be displayed, so maybe not the better for user) or by adding a new field.

edit-2:
after some trials I decided to be more radical by removing the needs from the needs_all_needs and the rendering node. I also tried to manage nested needs.
The unit tests shows that needs are excluded from needs.json and from directives which is the expected behavior (at least from y point of view ^^)
But from your explanations I guess I didn't handled well all the complexity behind ?

David-Le-Nir added a commit to ThalesGroup/sphinx-needs that referenced this issue Feb 12, 2024
David-Le-Nir added a commit to ThalesGroup/sphinx-needs that referenced this issue Feb 12, 2024
David-Le-Nir added a commit to ThalesGroup/sphinx-needs that referenced this issue Feb 12, 2024
David-Le-Nir added a commit to ThalesGroup/sphinx-needs that referenced this issue Feb 12, 2024
David-Le-Nir added a commit to ThalesGroup/sphinx-needs that referenced this issue Feb 12, 2024
David-Le-Nir added a commit to ThalesGroup/sphinx-needs that referenced this issue Feb 12, 2024
David-Le-Nir added a commit to ThalesGroup/sphinx-needs that referenced this issue Feb 12, 2024
David-Le-Nir added a commit to ThalesGroup/sphinx-needs that referenced this issue Feb 13, 2024
@danwos
Copy link
Member

danwos commented Feb 15, 2024

Will hopefully find some time to review the PR again soon.

In the meantime, yes if-builder is often used as an replacement for only, and in my experience it works much better.
But I wasn't aware only supports sphinx-tags, so that is a missing feature.

Maybe a small, new sphinx extension Sphinx-If could be the better solution. Taking the code from if-builder, extending it to support sphinx-tags, and it's done :) Supporting Sphinx-Needs objects, toctree and co. out of the box, by not even parsing not needed constructs.

Regarding edit-2: I guess this is the way to go. Only having the data inside needs.json, which shall be there. No magic boolean value, which needs to be known and handled correctly.

@David-Le-Nir
Copy link
Contributor Author

Maybe a small, new sphinx extension Sphinx-If could be the better solution. Taking the code from if-builder, extending it to support sphinx-tags, and it's done :)

Do you think we have to create a new, independent, sphinx extension of just improve the if-builder to accept tags ?
Maybe a new extension would be too much duplicating ? Or maybe if-builder and if-include should be entirely moved to sphinx-if ?

I have to discuss with my teammates but as only is an important feature for our users we could take time to create this directive. Or would you prefer to handle it on your own ?
(If we do it, it will be somehow a big copy-paste from your work)

@danwos
Copy link
Member

danwos commented Feb 15, 2024

Technically we could do it everywhere, but using an updated if-builder would always mean installing the complete sphinx-simpldpdf package. So 99% of code and dependencies would not be needed.

So, I guess a small standalone Sphinx extension would be much cleaner.
I would also then remove if-builder from Sphinx-SimplePDF one day and reference to the more generic solution.

And I would be grateful if you could support me here and create the initial extension.
For sure copying all the needed code is fully ok, and me and my small team can also support or add single features as PR, if there are special use cases from our existing packages.

@chrisjsewell chrisjsewell linked a pull request Feb 15, 2024 that will close this issue
@chrisjsewell chrisjsewell linked a pull request Feb 26, 2024 that will close this issue
@David-Le-Nir
Copy link
Contributor Author

Hello,

I implemented (for the moment locally in our gitlab, not on github) the "only-tags" and "only-builder" directives based on what was made in sphinx-simplepdf.
This works pretty well but I have an issue and maybe you could have an idea about how fixing it:

If I do for example:

.. only-builder:: text

   .. toctree::

      file_text.rst
      file_all.rst

.. only-builder:: html

   .. toctree::

      file_html.rst
      file_all.rst

I get the right toctrees and navigation links in the generated documents based on different builders.
However the "file_text" and "file_html" are always built whatever the chosen builder. Thus if I perform a search, I get them in the results and I'm able to navigate on it.

Do you have an idea of how I can exclude them from generation or else from search result ?

Regards

@danwos
Copy link
Member

danwos commented Apr 2, 2024

Argh, that may be tough, as Sphinx generates automatically all rst/md files it finds in the used source-dir, even if they are not part of a toctree.

I'm not sure if exlude_patterns could be set programmtically.

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