Copilot commented on code in PR #12588:
URL: https://github.com/apache/trafficserver/pull/12588#discussion_r2445718597
##########
tools/hrw4u/src/symbols.py:
##########
@@ -63,32 +63,31 @@ def declare_variable(self, name: str, type_name: str) ->
str:
def resolve_assignment(self, name: str, value: str, section: SectionType |
None = None) -> str:
with self.debug_context("resolve_assignment", name, value, section):
- for op_key, (commands, validator, uppercase, restricted_sections)
in self._operator_map.items():
+ for op_key, params in self._operator_map.items():
if op_key.endswith("."):
if name.startswith(op_key):
- self.validate_section_access(name, section,
restricted_sections)
+ self.validate_section_access(name, section,
params.sections if params else None)
qualifier = name[len(op_key):]
- if uppercase:
+ if params and params.upper:
qualifier = qualifier.upper()
- if validator:
- validator(qualifier)
+ if params and params.validate:
+ params.validate(qualifier)
- # Add boolean value validation for http.cntl
assignments.
+ # Add boolean value validation for http.cntl
assignments
if op_key == "http.cntl.":
types.SuffixGroup.BOOL_FIELDS.validate(value)
+ commands = params.target if params else None
if isinstance(commands, (list, tuple)):
- if value == '""':
- return f"{commands[0]} {qualifier}"
- else:
- return f"{commands[1]} {qualifier} {value}"
- else:
- return f"{commands} {qualifier} {value}"
+ return f"{commands[0 if value == '\"\"' else 1]}
{qualifier}" + ("" if value == '""' else f" {value}")
Review Comment:
This line contains duplicated conditional logic checking `value == '\"\"'`.
Extract the boolean to a variable for better readability and performance.
```suggestion
is_empty_quoted = value == '""'
return f"{commands[0 if is_empty_quoted else 1]}
{qualifier}" + ("" if is_empty_quoted else f" {value}")
```
##########
tools/hrw4u/src/lsp/hover.py:
##########
@@ -403,8 +403,10 @@ def get_operator_hover_info(operator: str) -> Dict[str,
Any]:
# Check exact matches first
if operator in OPERATOR_MAP:
- commands, _, is_prefix, sections = OPERATOR_MAP[operator]
- cmd_str = commands if isinstance(commands, str) else ' /
'.join(commands)
+ params = OPERATOR_MAP[operator]
+ commands = params.target if params else None
+ cmd_str = commands if isinstance(commands, str) else ' /
'.join(commands) if commands else "unknown"
Review Comment:
[nitpick] The nested ternary operators reduce readability. Consider using
explicit if-elif-else statements or extract to a helper function.
```suggestion
if isinstance(commands, str):
cmd_str = commands
elif commands:
cmd_str = ' / '.join(commands)
else:
cmd_str = "unknown"
```
##########
tools/hrw4u/src/types.py:
##########
@@ -166,3 +170,73 @@ def as_cond(self) -> str:
def as_operator(self, value: str) -> str:
return f"{self.var_type.op_tag} {self.index} {value}"
+
+
+class MapParams:
+ """Map parameters for table entries combining flags and metadata.
+ """
+
+ def __init__(
+ self,
+ upper: bool = False,
+ add: bool = False,
+ prefix: bool = False,
+ validate: Callable[[str], None] | None = None,
+ sections: set[SectionType] | None = None,
+ rev: dict | None = None,
+ target: str | list[str] | tuple[str, ...] | None = None) -> None:
+ object.__setattr__(
+ self, '_params', {
+ 'upper': upper,
+ 'add': add,
+ 'prefix': prefix,
+ 'validate': validate,
+ 'sections': sections,
+ 'rev': rev,
+ 'target': target
+ })
+
+ def __getattr__(self, name: str) -> bool | Callable[[str], None] |
set[SectionType] | dict | str | None:
Review Comment:
[nitpick] The return type union is overly complex and difficult to maintain.
Consider using `typing.Union` with a type alias or returning `Any` with better
documentation to explain what each attribute returns.
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -279,12 +284,15 @@ def _handle_statement_function(self, name: str, args:
list[str], section: Sectio
qargs = [status_code,
self._rewrite_inline_percents(f'"{url_arg}"', section)]
elif name == "add-header" and args:
+ # Use += operator syntax for add-header
Review Comment:
The comment states 'Use += operator syntax for add-header' but the code
below returns the += format. Update the comment to clarify this converts
add-header to += syntax for reverse mapping.
```suggestion
# Convert add-header to += syntax for reverse mapping
```
##########
tools/hrw4u/src/types.py:
##########
@@ -166,3 +170,73 @@ def as_cond(self) -> str:
def as_operator(self, value: str) -> str:
return f"{self.var_type.op_tag} {self.index} {value}"
+
+
+class MapParams:
+ """Map parameters for table entries combining flags and metadata.
+ """
+
+ def __init__(
+ self,
+ upper: bool = False,
+ add: bool = False,
+ prefix: bool = False,
+ validate: Callable[[str], None] | None = None,
+ sections: set[SectionType] | None = None,
+ rev: dict | None = None,
+ target: str | list[str] | tuple[str, ...] | None = None) -> None:
+ object.__setattr__(
+ self, '_params', {
+ 'upper': upper,
+ 'add': add,
+ 'prefix': prefix,
+ 'validate': validate,
+ 'sections': sections,
+ 'rev': rev,
+ 'target': target
+ })
+
+ def __getattr__(self, name: str) -> bool | Callable[[str], None] |
set[SectionType] | dict | str | None:
+ if name.startswith('_'):
+ raise AttributeError(f"'{type(self).__name__}' object has no
attribute '{name}'")
+ return self._params.get(name, False if name in ('upper', 'add',
'prefix') else None)
+
+ def __setattr__(self, name: str, value: object) -> None:
+ """Prevent modification after initialization (immutable)."""
+ raise AttributeError(f"'{type(self).__name__}' object is immutable")
+
+ def __repr__(self) -> str:
+ non_defaults = []
+ for k, v in self._params.items():
+ if k in ('upper', 'add', 'prefix'):
+ if v:
+ non_defaults.append(f"{k}=True")
+ elif v is not None:
+ if isinstance(v, set):
+ non_defaults.append(f"{k}={{{', '.join(str(s) for s in
v)}}}")
+ elif k in ('validate',):
Review Comment:
The tuple with a single element `('validate',)` is unnecessarily wrapped.
Use `k == 'validate'` instead for clarity.
```suggestion
elif k == 'validate':
```
--
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]
