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

ifconfig: Add a meta node to fix iteration #10502

Merged
merged 4 commits into from Jun 2, 2022

Conversation

AA-Turner
Copy link
Member

Fixes #10496

This issue arose due to the change from .traverse to .findall, which moved from a precompiled list to a generator.

Feature or Bugfix

  • Bugfix

A

@AA-Turner
Copy link
Member Author

cc: @djhoese can you test this please and see if it works?

@AA-Turner
Copy link
Member Author

3.11 failing due to pytest-dev/pytest#10008

A

@djhoese
Copy link

djhoese commented May 31, 2022

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.

@@ -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())
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

@AA-Turner
Copy link
Member Author

@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

@tk0miya
Copy link
Member

tk0miya commented Jun 2, 2022

Now I pushed "wrapping" code. Merging now.

@tk0miya tk0miya merged commit ab58bba into sphinx-doc:5.0.x Jun 2, 2022
@AA-Turner AA-Turner deleted the ifconfig-fix branch June 16, 2022 22:19
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants