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

format_signatures: Fix whitespace issues and add tests #381

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jbms
Copy link
Owner

@jbms jbms commented Aug 6, 2024

No description provided.

@jbms jbms force-pushed the fix-format-signatures-space branch from 8a8beba to d58344e Compare August 6, 2024 19:16
@jbms jbms requested a review from 2bndy5 August 6, 2024 19:16
@jbms jbms force-pushed the fix-format-signatures-space branch 2 times, most recently from b0b77a7 to 31c4c84 Compare August 6, 2024 19:51
@jbms jbms force-pushed the fix-format-signatures-space branch from 31c4c84 to af2303c Compare August 6, 2024 21:41
@@ -151,6 +151,9 @@ def _transform_node(node):
if isinstance(node, docutils.nodes.Text):
return replacements.get(id(node)) or []
new_node = node.copy()
# Ensure the new node does not contain any children. `desc_sig_space`,
# for example, adds a default child text node with content " ".
del new_node[:]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this be better?

Suggested change
del new_node[:]
new_node.children.clear()

I think the current change would delete children and attributes, no?

Comment on lines +197 to +218
first_name_text_node: Optional[docutils.nodes.Text] = None
for adjusted_node in adjusted_nodes:
node: docutils.nodes.Element
for node in adjusted_node.findall(
lambda node: (
isinstance(node, sphinx.addnodes.desc_addname)
or (
isinstance(node, sphinx.addnodes.desc_name)
and "sig-name-nonprimary" not in node["classes"]
)
)
):
for text_node in cast(docutils.nodes.Element, node).findall(
docutils.nodes.Text
):
first_name_text_node = text_node
break
if first_name_text_node is not None:
break
if first_name_text_node is not None:
break
return first_name_text_node
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why all the breaks? Seems like a simple return makes sense.

Suggested change
first_name_text_node: Optional[docutils.nodes.Text] = None
for adjusted_node in adjusted_nodes:
node: docutils.nodes.Element
for node in adjusted_node.findall(
lambda node: (
isinstance(node, sphinx.addnodes.desc_addname)
or (
isinstance(node, sphinx.addnodes.desc_name)
and "sig-name-nonprimary" not in node["classes"]
)
)
):
for text_node in cast(docutils.nodes.Element, node).findall(
docutils.nodes.Text
):
first_name_text_node = text_node
break
if first_name_text_node is not None:
break
if first_name_text_node is not None:
break
return first_name_text_node
for adjusted_node in adjusted_nodes:
node: docutils.nodes.Element
for node in adjusted_node.findall(
lambda node: (
isinstance(node, sphinx.addnodes.desc_addname)
or (
isinstance(node, sphinx.addnodes.desc_name)
and "sig-name-nonprimary" not in node["classes"]
)
)
):
for text_node in cast(docutils.nodes.Element, node).findall(
docutils.nodes.Text
):
return text_node
return None

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

Successfully merging this pull request may close these issues.

2 participants