Copilot commented on code in PR #53367:
URL: https://github.com/apache/airflow/pull/53367#discussion_r2207143525
##########
devel-common/src/sphinx_exts/substitution_extensions.py:
##########
@@ -61,28 +61,28 @@ def condition(node):
return isinstance(node, (nodes.literal_block, nodes.literal))
for node in self.document.traverse(condition):
- if not node.get(_SUBSTITUTION_OPTION_NAME):
- continue
-
- # Some nodes don't have a direct document property, so walk up
until we find it
- document = node.document
- parent = node.parent
- while document is None:
- parent = parent.parent
- document = parent.document
-
- substitution_defs = document.substitution_defs
- for child in node.children:
- old_child = child
- for name, value in substitution_defs.items():
- replacement = value.astext()
+ if isinstance(node, nodes.Element):
Review Comment:
Nodes that are not instances of `nodes.Element` will fall through and
execute substitution logic unexpectedly. Add an explicit `continue` for
non-`Element` nodes or refactor the guard to `if not isinstance(node,
nodes.Element) or _SUBSTITUTION_OPTION_NAME not in node.attributes: continue`
to restore the original behavior.
##########
devel-common/src/sphinx_exts/substitution_extensions.py:
##########
@@ -61,28 +61,28 @@ def condition(node):
return isinstance(node, (nodes.literal_block, nodes.literal))
for node in self.document.traverse(condition):
- if not node.get(_SUBSTITUTION_OPTION_NAME):
- continue
-
- # Some nodes don't have a direct document property, so walk up
until we find it
- document = node.document
- parent = node.parent
- while document is None:
- parent = parent.parent
- document = parent.document
-
- substitution_defs = document.substitution_defs
- for child in node.children:
- old_child = child
- for name, value in substitution_defs.items():
- replacement = value.astext()
+ if isinstance(node, nodes.Element):
+ if _SUBSTITUTION_OPTION_NAME not in node.attributes:
Review Comment:
The original `node.get(...)` check skipped nodes where the attribute is
present but falsey. The new check only tests for existence, not truthiness. Use
`if not node.attributes.get(_SUBSTITUTION_OPTION_NAME):` to preserve the
original semantics.
```suggestion
if not node.attributes.get(_SUBSTITUTION_OPTION_NAME):
```
##########
devel-common/src/sphinx_exts/substitution_extensions.py:
##########
@@ -61,28 +61,28 @@ def condition(node):
return isinstance(node, (nodes.literal_block, nodes.literal))
for node in self.document.traverse(condition):
- if not node.get(_SUBSTITUTION_OPTION_NAME):
- continue
-
- # Some nodes don't have a direct document property, so walk up
until we find it
- document = node.document
- parent = node.parent
- while document is None:
- parent = parent.parent
- document = parent.document
-
- substitution_defs = document.substitution_defs
- for child in node.children:
- old_child = child
- for name, value in substitution_defs.items():
- replacement = value.astext()
+ if isinstance(node, nodes.Element):
+ if _SUBSTITUTION_OPTION_NAME not in node.attributes:
+ continue
+
+ # Some nodes don't have a direct document property, so walk up
until we find it
+ document = node.document
+ parent = node.parent
+ while document is None:
+ parent = parent.parent
+ document = parent.document
+
+ substitution_defs = document.substitution_defs
+ for child in node.children:
+ old_child = child
if isinstance(child, nodes.Text):
Review Comment:
Calling `node.replace(old_child, child)` unconditionally—even when `child`
is unchanged—can result in unnecessary work. Consider only invoking `replace`
when the text actually changes or wrapping it inside the `if` block.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
