Conversation
| pass | ||
|
|
||
| def replace_table(self, label: str, data) -> None: | ||
| def replace_table(self, label: str, data, header=False, rownames=False): |
There was a problem hiding this comment.
We allow use kwargs only for header and rownames:
def replace_table(self, label: str, data, *, header=False, rownames=False):In my opinion it's better to use None instead of False. When using False as default value I assume a Boolean value is required. None is no valid data fore these arguments and therefore it should be fine to use it. Using an Undefined() object could be an option, too. But I think this is not necessary in this case.
lysnikolaou
left a comment
lysnikolaou
left a comment
There was a problem hiding this comment.
One minor comment for _find_shapes. Other that that, I think, tests should definitely be included before merging this.
| pass | ||
| assert isinstance(data, pandas.dataframe) | ||
|
|
||
| shapes_to_replace = self._find_shapes(label) |
There was a problem hiding this comment.
In #7, the method signature of _find_shapes was changed? Maybe change this to reflect that?
| data (pandas.DataFrame): table to be inserted into the presentation | ||
| """ | ||
| pass | ||
| assert isinstance(data, pandas.dataframe) |
There was a problem hiding this comment.
You could remove the assertion here and include type hints in the method signature like so:
def replace_table(self, label: str, data: pandas.dataframe, *, header: bool = False, rownames: bool = False) -> None:3 participants
Hi,
Adding my replace_table() from yesterday. Tests not yet created.
Best,
Martin