change list command output from tabulate to rich tables #359
Conversation
|
|
|
@Haris-Ali007: what's the status here? before I can merge this, at least the CI complaints must be fixed... |
Yes apologies for the delay. I am fixing the issues. |
odxtools/cli/_print_utils.py
Outdated
| value_type: List[Optional[str]] = [] | ||
| data_type: List[Optional[str]] = [] | ||
| dop: List[Optional[str]] = [] | ||
| name: List[Optional[object]] = [] |
There was a problem hiding this comment.
you can use just list instead of List[Optional[object]]
There was a problem hiding this comment.
I defined it as object to solve the cli issue. So you say I keep it simply like name = [ ]?
There was a problem hiding this comment.
I am just proposing a simpler syntax, yours work if you want to keep it
There was a problem hiding this comment.
okay sure thank you.
There was a problem hiding this comment.
AFAICS this code is still unchanged? But actually I prefer to use List[Any] here, or -- even better -- replacing the Any by the types which can actually occur here. Since the latter might be quite a bit of work, Any is okay for now...
There was a problem hiding this comment.
hm, isn't the name column non-optional and always a string, i.e., List[str]?
There was a problem hiding this comment.
Yeah. you are right. let me fix this.
There was a problem hiding this comment.
@andlaus I have made changes, I kept the List[Any] for two variables because here I was getting static type error if defined as int, str or None.
odxtools/cli/_print_utils.py
Outdated
| byte: List[Any] = [] | ||
| bit_length: List[Any] = [] |
There was a problem hiding this comment.
that should probably be
| byte: List[Any] = [] | |
| bit_length: List[Any] = [] | |
| byte: List[Optional[int]] = [] | |
| bit_length: List[Optional[int]] = [] |
?
odxtools/cli/_print_utils.py
Outdated
| @@ -228,9 +228,10 @@ def extract_parameter_tabulation_data(parameters: List[Parameter]) -> Table: | |||
| value_type.append(None) | |||
| dop.append(None) | |||
|
|
|||
| for lst in [byte, semantic, bit_length, value, value_type, data_type, dop]: | |||
| lst[:] = ["" if x is None else x for x in lst] | |||
There was a problem hiding this comment.
great that you made this code here more readably. line 232 should probably better be
| lst[:] = ["" if x is None else x for x in lst] | |
| # replace `None` entries by empty strings | |
| lst = ["" if x is None else x for x in lst] |
There was a problem hiding this comment.
I have tried using simple lst it doesn't override the list in-place.
There was a problem hiding this comment.
why does it need to be overwritten in-place? (memory wise it should not make any difference because the current solution creates a temporary list as well and also the required memory hopefully is miniscule...)
There was a problem hiding this comment.
Yes it does create a temporary list in both case, but in case of list[:] it copies the temporary list to the original address. So in a way I am able to edit the list in it's place.
There was a problem hiding this comment.
okay, I had a hands-on look and I now understand why the in-place copy of the list is necessary. That said, declaring List[Any] for any of the variables above causes mypy to give up with these variables altogether, so -- while not optimal either -- I prefer to use List[Optional[int]] above and add a # type: ignore[attr-defined, index] comment here...
There was a problem hiding this comment.
okay thanks for the that. I am new to this static type checks. I'll push the changes
|
all good now. merging, thanks for your work :) |
…z#359) * change list command output from tabulate to rich tables * Add indentation to rich tables * fix coding style errors * Resolve static type checking error in _print_utils.py * Update browse.py from tabulate to rich table * Remove commented code * Fix lint coding style issues * Fix lint code quality issue * Fix static type definition for paramters * Fix static type definitions for variables defined as List[Any]
Issue resolved: I have updated the list bash output by replacing the tabulate table with rich tables.
File updated: cli/_print_utils.py
The output looks like this:
