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.

Sign up for GitHub

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

Jump to bottom

_check_toc_parents should consider only the descendants of root_doc and n… #13038

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+27 −7
Open

_check_toc_parents should consider only the descendants of root_doc and n… #13038

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f371bbb
`_check_toc_parents` considers only the descendants of root_doc and n…
khanxmetu Oct 18, 2024
cc5f3f8
Code refactoring suggestion
khanxmetu Oct 19, 2024
26b216e
Keep parents list as unsorted
khanxmetu Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions sphinx/environment/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ def check_consistency(self) -> None:
# Call _check_toc_parents here rather than in _get_toctree_ancestors()
# because that method is called multiple times per document and would
# lead to duplicate warnings.
_check_toc_parents(self.toctree_includes)
_check_toc_parents(self.toctree_includes, self.config.root_doc)

# call check-consistency for all extensions
self.domains._check_consistency()
Expand Down Expand Up @@ -819,19 +819,28 @@ def _traverse_toctree(
traversed.add(sub_docname)


def _check_toc_parents(toctree_includes: dict[str, list[str]]) -> None:
def _check_toc_parents(toctree_includes: dict[str, list[str]], root_doc: str) -> None:
"""Checks if document is referenced in multiple toctrees.
Based on the current implementation of `global_toctree_for_doc`,
it considers only the descendants of root_doc and not the whole graph.
"""
toc_parents: dict[str, list[str]] = {}
for parent, children in toctree_includes.items():
for child in children:
toc_parents.setdefault(child, []).append(parent)

def _find_toc_parents_dfs(node: str) -> None:
for child in toctree_includes.get(node, []):
toc_parents.setdefault(child, []).append(node)
is_child_already_visited = len(toc_parents[child]) > 1
if not is_child_already_visited:
_find_toc_parents_dfs(child)
khanxmetu marked this conversation as resolved.
Show resolved Hide resolved

_find_toc_parents_dfs(root_doc)
for doc, parents in sorted(toc_parents.items()):
Copy link
Contributor

Choose a reason for hiding this comment

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

A personal opinion: the code might be easier to understand if the iteration source of the subsequent for loop -- toc_parents -- is assigned-to from the result of the _find_toc_parents_dfs function.

Explaining why: to me, function calls that have side-effects that affect outer-scoped variables are slightly hard to follow.

I think that another potential benefit could be that it'd be easier to write test coverage for the helper function (although I admit that it's a small one, and that perhaps the enclosing function is a better candidate for testing here).

Copy link
Contributor Author

@khanxmetu khanxmetu Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A personal opinion: the code might be easier to understand if the iteration source of the subsequent for loop -- toc_parents -- is assigned-to from the result of the _find_toc_parents_dfs function.

Explaining why: to me, function calls that have side-effects that affect outer-scoped variables are slightly hard to follow.

I think that another potential benefit could be that it'd be easier to write test coverage for the helper function (although I admit that it's a small one, and that perhaps the enclosing function is a better candidate for testing here).

I didn't worry too much about side-effects as it being more simplistic this way.

Here is the DFS without side-effect:

    def _find_toc_parents_dfs(node: str, toc_parents: dict[str, list[str]] = {}) -> dict[str, list[str]]:
        for child in toctree_includes.get(node, []):
            already_visited = child in toc_parents
            toc_parents.setdefault(child, []).append(node)
            if already_visited:
                continue
            _find_toc_parents_dfs(child, toc_parents)
        return toc_parents

Personally I found it slightly more complicated than needed because of toc_parents being propogated down the tree as a parameter but also being returned. Note that return toc_parents will only be used by the external caller of the helper function and not elsewhere.
Anyways I'm fine with this implementation too if you think so.

Edit: There exists another DFS implementation, without taking toc_parents dict as a parameter but only relying on return values, however I believe that would require combining the returned dicts from each subtree at each node which would be expensive.

if len(parents) > 1:
logger.info(
__(
'document is referenced in multiple toctrees: %s, selecting: %s <- %s'
),
parents,
sorted(parents),
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the sorting added here for debugging/investigation purposes? (and should we include it with these changes?)

Copy link
Contributor Author

@khanxmetu khanxmetu Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The helper function uses preorder traversal which does not guarantee sorted parents as was before. Sorting is kept for consistency reasons (independent of the helper function traversal order) in the logged output, this way it is also easier to write the corresponding tests instead of dry running the traversal order and depending on the helper functions implementation. Further, it also makes it easier for the user to spot the pattern that the lexicographically greatest parent is being selected.

Copy link
Contributor

Choose a reason for hiding this comment

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

My initial sense here is that I'm not too keen on the practice of modifying application code in order to make test expectations easier to write.

I do understand that it helps in this case, but I think that unit test coverage of different tree/graph structures would be more robust over time.

(apologies for taking a while to add further review commentary)

Copy link
Contributor Author

@khanxmetu khanxmetu Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My initial sense here is that I'm not too keen on the practice of modifying application code

I do not understand how the application code is modified since the function _check_toc_parents() does not produce any side-effects other than the output. In fact I think the idea of applying sorted is quite the opposite.
I'd like to clarify again that the mentioned benefits/reasons in my previous comment of having sorted parents aren't enforced by this PR, instead the parents were already implicitly sorted previously due to the node-wise traversal and the sorted nature of values in toctree_includes. Since the traversal order is now changed to inorder which doesn't inherently guarantee parents being collected in sorted order, sorted function is now applied post-traversal, to keep it consistent with the previous behavior. You could argue that guaranteeing the order of parents should come from the nested helper function itself and the outer body of check_toc_parents() shall not be modified, however given the recursive nature of the helper function, I feel like the current approach is much simpler.

I do understand that it helps in this case, but I think that unit test coverage of different tree/graph structures would be more robust over time.

I don’t have a strong opinion on writing unittests for helper functions, in my opinion we should be testing based on functionality and not the implementation of a function which in this current case would mean that the tests should only care about consistent warning/logging and not about whatever method of traversal is used internally to achieve so. For example the helper method which used node-wise traversal previously, and now inorder traversal in this PR, should ideally NOT break the existing tests and hence having a determined order of parents regardless of the traversal algorithm helps achieve it.

I’d like to know more about what you think of this. If you still believe that we shouldn’t guarantee sorted order of parents anymore, I’d happily remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

My initial sense here is that I'm not too keen on the practice of modifying application code

I do not understand how the application code is modified since the function _check_toc_parents() does not produce any side-effects other than the output.

Logging is sometime more than a side-effect, and it can either express or hide internal application state. Sometimes that's risky, and sometimes it's useful. In the context of trying to improve the consistency of build output, I think it's likely to be useful to log the iteration order of the parent document names without sorting applied to them.

As I understand it, commit 8351936 does sort the keys of the toctree includes -- but it doesn't sort the values (the parent document list).

Copy link
Contributor Author

@khanxmetu khanxmetu Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback, I'll remove sorting from here then.


As I understand it, commit 8351936 does sort the keys of the toctree includes -- but it doesn't sort the values (the parent document list).

Note that the toctree_includes is a parent -> children mapping. Essentially by sorting the keys (which are parent names) and later iterating over the dictionary, it is guaranteed that parents are processed in lexicographical order. Therefore when the toctree_includes is reversed to obtain toc_parents i.e child -> parents mapping in check_toc_parents, the parents list for each child key would already be sorted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the toctree_includes is a parent -> children mapping. Essentially by sorting the keys (which are parent names) and later iterating over the dictionary, it is guaranteed that parents are processed in lexicographical order. Therefore when the toctree_includes is reversed to obtain toc_parents i.e child -> parents mapping in check_toc_parents, the parents list for each child key would already be sorted.

Ok, this makes sense, thank you. I didn't understand this until I began attempting to write some unit tests locally. What I'd started on was a test to provide three or four permutations of the same graph -- sometimes with child value lists randomized, sometimes with parent keys randomized -- with the intent of asserting on consistent inverted-graph structure as output.

(in particular, refreshing my memory about the lexicographic sorting took me some time -- and now I understand from that plus your message here, that the check code relies on the parent-key order, and that without a larger refactoring, it wouldn't be valid to write test cases that randomize that ordering, because the application code itself shouldn't allow that to occur)

Copy link
Contributor Author

@khanxmetu khanxmetu Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

that the check code relies on the parent-key order, and that without a larger refactoring, it wouldn't be valid to write test cases that randomize that ordering, because the application code itself shouldn't allow that to occur

While it is true that the order of nodes visited during the reversal of the graph edges rely relied on the parent-key order from toctree_includes which is ensured to be deterministic from my previous related PR. However I cannot understand the motivation behind testing the randomized ordering, because in formal definitions a graph consists of set of edge pairs, this unordering implies that any permutation of parents lists in toc_parents values would correspond to the same graph by definition although different internal representations.

Was your idea about testing whether the toc_parents dictionary order obtained from the dfs is equivalent for any randomized order of key values of a specific graph of toctree_includes?

Edit: The dfs algorithm presented here relies on children-values ordering of toctree_includes instead which I don't think is always sorted.

max(parents),
doc,
location=doc,
Expand Down
7 changes: 7 additions & 0 deletions tests/roots/test-toctree-multiple-parents/pdfindex.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
test-toctree-multiple-parents
=============================

.. literalinclude:: relation_graph.txt

.. toctree::
delta
4 changes: 2 additions & 2 deletions tests/roots/test-toctree-multiple-parents/relation_graph.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
index
/ \
index pdfindex
/ \ /
alpha delta
\ /
bravo /
Expand Down
3 changes: 3 additions & 0 deletions tests/test_builders/test_build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ def test_multiple_parents_toctree(app):
assert (
"document is referenced in multiple toctrees: ['bravo', 'delta'], selecting: delta <- charlie"
) in app.status.getvalue()
assert (
"document is referenced in multiple toctrees: ['index', 'pdfindex'], selecting: pdfindex <- delta"
) not in app.status.getvalue()


@pytest.mark.usefixtures('_http_teapot')
Expand Down
Loading