Copilot commented on code in PR #13665:
URL: https://github.com/apache/trafficserver/pull/13665#discussion_r3976746401
##########
tools/hrw4u/src/ast_visitor.py:
##########
@@ -39,26 +55,26 @@ def visitProgram(self, ctx) -> HRW4UAST:
elif item.section() is not None:
items.append(self._visit_section(item.section()))
elif item.commentLine() is not None:
- pass
+ items.append(self._visit_comment(item.commentLine()))
else:
raise ValueError(f"Unhandled programItem alternative at line
{item.start.line}")
Review Comment:
The new AST model is file/line/column-aware via `Span`, but these internal
error messages still only mention `line`. Including `self.filename` and
`item.start.column` (and similarly for the other `Unhandled ... at line ...`
raises) would make failures much easier to pinpoint when parsing/inlining
across files.
##########
tools/hrw4u/src/ast_nodes.py:
##########
@@ -77,33 +81,37 @@ class RegexValue:
raw: str
-ValueExpr = Union[LiteralStringValue, IdentValue, IPValue, ParamRef, int,
bool, tuple[IPValue, ...]]
+@dataclass(frozen=True, kw_only=True)
+class SetValue:
+ """An `in [...]` operand. Emitted as `(raw)`, so the brackets are stripped
but quoting is not."""
+ raw: str
@dataclass(frozen=True, kw_only=True)
-class Node:
- line: int
+class IpRangeValue:
+ """An `in {...}` operand. Emitted verbatim, braces included."""
+ raw: str
-@dataclass(frozen=True)
-class Target:
- namespace: str | None
- field: str
+ValueExpr = Union[LiteralStringValue, IdentValue, IPValue, ParamRef, int,
bool, IpRangeValue]
Review Comment:
The typing/modeling of membership RHS is asymmetric: `IpRangeValue` is
treated as a general `ValueExpr`, while `SetValue` is a separate special-case
type in `Comparison.right`. If these are both membership-only operands,
consider introducing a shared type (e.g., `MembershipValue = SetValue |
IpRangeValue`) and using that in `Comparison.right` (and in the visitor return
hints). This keeps the API surface more uniform and reduces “why is iprange a
value but set is not?” confusion for future maintainers.
##########
tools/hrw4u/src/ast_nodes.py:
##########
@@ -119,11 +127,16 @@ class Break(Node):
pass
+@dataclass(frozen=True, kw_only=True)
+class Comment(Node):
+ text: str
+
+
@dataclass(frozen=True, kw_only=True)
class Comparison(Node):
left: IdentValue | FunctionCall
operator: str # "==", "!=", ">", "<", "~", "!~", "in", "!in"
- right: ValueExpr | RegexValue | tuple[ValueExpr, ...]
+ right: ValueExpr | RegexValue | SetValue
modifiers: tuple[str, ...]
Review Comment:
The typing/modeling of membership RHS is asymmetric: `IpRangeValue` is
treated as a general `ValueExpr`, while `SetValue` is a separate special-case
type in `Comparison.right`. If these are both membership-only operands,
consider introducing a shared type (e.g., `MembershipValue = SetValue |
IpRangeValue`) and using that in `Comparison.right` (and in the visitor return
hints). This keeps the API surface more uniform and reduces “why is iprange a
value but set is not?” confusion for future maintainers.
##########
tools/hrw4u/tests/test_ast_nodes.py:
##########
@@ -15,27 +15,31 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-from hrw4u.ast_nodes import Target
+import pytest
+from hrw4u.ast_nodes import Span
-class TestTarget:
- def test_dotted_path(self):
- t = Target.from_dotted("inbound.req.X-Foo")
- assert t.namespace == "inbound.req"
- assert t.field == "X-Foo"
+class TestSpan:
- def test_two_segments(self):
- t = Target.from_dotted("inbound.ip")
- assert t.namespace == "inbound"
- assert t.field == "ip"
+ def test_equality_is_by_value(self):
+ assert Span(file="a", line=1, column=0) == Span(file="a", line=1,
column=0)
+ assert Span(file="a", line=1, column=0) != Span(file="b", line=1,
column=0)
- def test_no_dots(self):
- t = Target.from_dotted("bool_0")
- assert t.namespace is None
- assert t.field == "bool_0"
+ def test_is_hashable_so_it_can_key_a_span_index(self):
+ first = Span(file="a", line=1, column=0)
+ assert {first: "node"}[Span(file="a", line=1, column=0)] == "node"
- def test_deep_namespace(self):
- t = Target.from_dotted("http.cntl.TXN_DEBUG")
- assert t.namespace == "http.cntl"
- assert t.field == "TXN_DEBUG"
+ def test_is_immutable(self):
+ span = Span(file="a", line=1, column=0)
+ with pytest.raises(Exception):
+ span.line = 2
Review Comment:
Catching a broad `Exception` can let unrelated failures pass (e.g., if an
unexpected runtime error is raised). Since `Span` is a frozen dataclass, this
can be made stricter by asserting the specific exception type raised for frozen
instances (from `dataclasses`), which improves signal if the dataclass
configuration changes.
##########
tools/hrw4u/tests/test_ast_nodes.py:
##########
@@ -15,27 +15,31 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-from hrw4u.ast_nodes import Target
+import pytest
+from hrw4u.ast_nodes import Span
-class TestTarget:
- def test_dotted_path(self):
- t = Target.from_dotted("inbound.req.X-Foo")
- assert t.namespace == "inbound.req"
- assert t.field == "X-Foo"
+class TestSpan:
- def test_two_segments(self):
- t = Target.from_dotted("inbound.ip")
- assert t.namespace == "inbound"
- assert t.field == "ip"
+ def test_equality_is_by_value(self):
+ assert Span(file="a", line=1, column=0) == Span(file="a", line=1,
column=0)
+ assert Span(file="a", line=1, column=0) != Span(file="b", line=1,
column=0)
- def test_no_dots(self):
- t = Target.from_dotted("bool_0")
- assert t.namespace is None
- assert t.field == "bool_0"
+ def test_is_hashable_so_it_can_key_a_span_index(self):
+ first = Span(file="a", line=1, column=0)
+ assert {first: "node"}[Span(file="a", line=1, column=0)] == "node"
- def test_deep_namespace(self):
- t = Target.from_dotted("http.cntl.TXN_DEBUG")
- assert t.namespace == "http.cntl"
- assert t.field == "TXN_DEBUG"
+ def test_is_immutable(self):
+ span = Span(file="a", line=1, column=0)
+ with pytest.raises(Exception):
+ span.line = 2
+
+ def test_rejects_unknown_attributes(self):
+ # slots=True: a typo'd field must fail loudly rather than sit unread
on the instance.
+ with pytest.raises(AttributeError):
+ Span(file="a", line=1, column=0).lineno = 2
+
+
+if __name__ == "__main__":
+ pytest.main([__file__, "-v"])
Review Comment:
This `__main__` runner is unusual in repository test files and can create
inconsistent execution paths vs the normal test runner (and can confuse tooling
that imports tests). If there isn’t a strong repo convention for these blocks,
it’s typically better to remove it and rely on the standard test invocation.
--
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]
