ifconfig: Add a meta node to fix iteration
#10502
Conversation
|
cc: @djhoese can you test this please and see if it works? |
|
3.11 failing due to pytest-dev/pytest#10008 A |
|
It works! Thanks. I tested it on both my reproducer in the related issue and on my actual project and both build and look correct. |
sphinx/ext/ifconfig.py
Outdated
| @@ -64,7 +65,7 @@ def process_ifconfig_nodes(app: Sphinx, doctree: nodes.document, docname: str) - | |||
| node.replace_self(newnode) | |||
| else: | |||
| if not res: | |||
| node.replace_self([]) | |||
| node.replace_self(addnodes.meta()) | |||
There was a problem hiding this comment.
Why do you insert a meta node? If my understanding is correct, it's better to convert the result of findall() to the list on the top of the loop, instead?
There was a problem hiding this comment.
Converting to a list is the option of last resort as it is slower (double iteration) and takes more memory -- a meta node here has no effect on the output and in effect is a noop node. I could convert to a list if you'd prefer, but this is the reason I didn't.
A
There was a problem hiding this comment.
I think doctrees should follow the semantics. So -1 for using meaningful node for the noop purpose. If you'd like to insert a noop node, how about using Text('') instead? It's meaningless and noop node. I can accept it.
|
@tk0miya I didn't have time to update this one, please feel free to push to this branch wrapping the findall code in a list, I think that is better than inserting a blank text node. A |
|
Now I pushed "wrapping" code. Merging now. |
Fixes #10496
This issue arose due to the change from
.traverseto.findall, which moved from a precompiled list to a generator.Feature or Bugfix
A