gemini-code-assist[bot] commented on code in PR #94:
URL: https://github.com/apache/tvm-ffi/pull/94#discussion_r2412315491
##########
python/tvm_ffi/cython/type_info.pxi:
##########
@@ -136,6 +126,29 @@ class TypeSchema:
def from_json_str(s) -> "TypeSchema":
return TypeSchema.from_json_obj(json.loads(s))
+ def repr(self, ty_map = None) -> str:
+ if ty_map is None:
+ origin = self.origin
+ else:
+ origin = ty_map(self.origin)
+ args = [i.repr(ty_map) for i in self.args]
+ if origin == "Union":
+ return " | ".join(args)
+ elif origin == "Optional":
+ return args[0] + " | None"
+ elif origin == "Callable":
+ if not args:
+ return "Callable[..., Any]"
+ else:
+ ret = args[0]
+ args = ", ".join(args[1:])
+ return f"Callable[[{args}], {ret}]"
+ elif not args:
+ return origin
+ else:
+ args = ", ".join(args)
+ return f"{origin}[{args}]"
Review Comment:
This new `repr` method is a great addition. A few suggestions to improve its
clarity and maintainability:
1. Add a type hint for the `ty_map` argument. This improves readability and
helps static analysis tools. Since `Callable` is not imported in this file, you
can use a string literal for the hint.
2. The variable `args` is reused and rebound to different types (a list of
strings, then a single string). This can be confusing. Using more descriptive,
distinct variable names for the list of representations and the joined string
would make the logic easier to follow.
```
def repr(self, ty_map: "Optional[Callable[[str], str]]" = None) -> str:
if ty_map is None:
origin = self.origin
else:
origin = ty_map(self.origin)
arg_reprs = [i.repr(ty_map) for i in self.args]
if origin == "Union":
return " | ".join(arg_reprs)
elif origin == "Optional":
return arg_reprs[0] + " | None"
elif origin == "Callable":
if not arg_reprs:
return "Callable[..., Any]"
else:
ret_repr = arg_reprs[0]
params_str = ", ".join(arg_reprs[1:])
return f"Callable[[{params_str}], {ret_repr}]"
elif not arg_reprs:
return origin
else:
args_str = ", ".join(arg_reprs)
return f"{origin}[{args_str}]"
```
##########
python/tvm_ffi/cython/type_info.pxi:
##########
@@ -86,7 +86,7 @@ _TYPE_SCHEMA_ORIGIN_CONVERTER = {
}
[email protected](repr=False, frozen=True)
[email protected](repr=False)
Review Comment:
Removing `frozen=True` makes `TypeSchema` instances mutable, which might be
an unintended side effect and could lead to bugs if instances are shared and
modified unexpectedly. It's preferable to keep data structures like this
immutable.
You can restore `frozen=True` here and use `object.__setattr__` in
`__post_init__` to set the default arguments for `list` and `dict` on lines
104-105 and 108-109. For example:
```python
# line 105
object.__setattr__(self, "args", (TypeSchema("Any"),))
# line 109
object.__setattr__(self, "args", (TypeSchema("Any"), TypeSchema("Any")))
```
```
@dataclasses.dataclass(repr=False, frozen=True)
```
##########
tests/python/test_metadata.py:
##########
@@ -67,6 +76,13 @@ def test_schema_global_func(func_name: str, expected: str)
-> None:
metadata: dict[str, Any] = get_global_func_metadata(func_name)
actual: TypeSchema = TypeSchema.from_json_str(metadata["type_schema"])
assert str(actual) == expected, f"{func_name}: {actual}"
+ assert actual.repr(_replace_list_dict) == expected.replace(
+ "list",
+ "Sequence",
+ ).replace(
+ "dict",
+ "Mapping",
+ )
Review Comment:
This assertion block for testing `actual.repr` is repeated in
`test_schema_field` (lines 114-120) and `test_schema_member_method` (lines
145-151). To improve maintainability and reduce code duplication, consider
extracting this logic into a shared helper function. For example:
```python
def check_schema_repr(actual: TypeSchema, expected: str, name: str):
assert str(actual) == expected, f"{name}: {actual}"
expected_replaced = expected.replace("list", "Sequence").replace("dict",
"Mapping")
assert actual.repr(_replace_list_dict) == expected_replaced, f"{name}
with ty_map: {actual.repr(_replace_list_dict)}"
```
You could then replace the two assertions in each test with a single call to
this helper.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
