`_check_toc_parents` should consider only the descendants of root_doc and n…

[khanxmetu]

…ot the whole toctree_includes graph

Purpose

• Skips the document referenced in multiple toctree warning/log when the said toctrees are not the descendants of root_doc toctree.
• The warning/log output now aligns with the current implementation of global_toctree_for_doc and _get_toctree_ancestors (i.e root_doc being hardcoded as the root ancestor for every document).

Relates

• Removes the inconsistent logged output: "document is referenced in multiple toctrees..." in 12999#discussioncomment-10909269
• Solution based on my comment 12999#discussioncomment-10921512

[jayaddison]

@khanxmetu a few things about this are still bothering me:

• I continue to believe we should be unit testing the results of tree/graph processing, not checking for presence/absence of log messages.
• Following on from my suggestion to remove one sorted(...) call -- I'm beginning to wonder whether this one is equally dubious.
• The _check_toc_parents method seems to be a duplicate implementation of the toc-parent resolution elsewhere in the code -- so they risk diverging in future (in fact, it makes it difficult to confirm that they haven't diverged already).

And perhaps a larger design question/concern about the bugreport:

This behavior seems like a bug since for documents included in the orphaned tree but not in master_doc, the sidebar navigation tree on that html document page still shows the toctree for master_doc and is unable to expand to the current document.

How to Reproduce

Toctree Graph

  index      orphan_root         
      \       /     \
       \    /        \ 
      alpha        bravo
       /              /
    delta       charlie

Do you mean that on the page for bravo, we display a toctree that presents content related to the index / alpha / etc side of the tree? If so, I think we should confirm whether that's what we want. I would personally expect that we'd present a toctree grounded in the part of the graph that we're currently in.

[jayaddison]

And perhaps a larger design question/concern about the bugreport:

Sorry, I didn't phrase this well - my worry there isn't about your bugreport - I'm grateful you're helping to investigate this. My worry is that the application itself may already be doing something we don't want -- and if so, adding more code that reinforces that unexpected behaviour could make it more difficult to repair properly in future. So my concern is to try to check that we're choosing the correct path before we make additional changes.

[khanxmetu]

I continue to believe we should be unit testing the results of tree/graph processing, not checking for presence/absence of log messages.

Initially I used test_circular_toctree() as a reference to write tests for this as the warning/checks was of somewhat similar nature. Since you asked, I'll make sure to change to a unittest testing the traversal only instead.

Following on from my suggestion to remove one sorted(...) call -- I'm beginning to wonder whether this one is equally dubious.

The idea was similar, yes. Since many parts of the codebase would sort the documents before processing or logging, I added this here too. I'll remove it due to the reasons you mentioned.

The _check_toc_parents method seems to be a duplicate implementation of the toc-parent resolution elsewhere in the code -- so they risk diverging in future (in fact, it makes it difficult to confirm that they haven't diverged already).

Could you elaborate on this please?

[khanxmetu]

Do you mean that on the page for bravo, we display a toctree that presents content related to the index / alpha / etc side of the tree? If so, I think we should confirm whether that's what we want. I would personally expect that we'd present a toctree grounded in the part of the graph that we're currently in.

In a perfect scenario, I believe that on bravo page the global toctree resolution in the side bar should begin from orphan_root and expanded to bravo, since it's more intuitive to believe that the tree would rooted at orphan_root and has no connection with index.

However this presents more complications.

Firstly, due to the hardcoded master_doc (index) in the existing codebase which would require some parts/functions to be rewritten to remove that dependency.

Secondly, if master_doc isn't set to be fixed as the root ancestor for any toctree, this PR would not have been possible as it would be virtually impossible to disregard the multiple toctree references from nodes that are not descendants of master_doc.

In conclusion, the mentioned bug isn't intended to be resolved by this PR, rather I documented it for the sake of record/documenting the issue.

[jayaddison]

Thank you again @khanxmetu for your responses. I think I may need to take a break soon from working on Sphinx -- I will try to review this again, but if I do not, then my apologies in advance and best of luck progressing it to merge readiness.