BUG: DataFrame.to_html validates formatters has the correct length

[guipleite]

• closes DataFrame.to_html should validate that formatters has the correct length #28469
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

@gabriellm1 @hugoecarl

[WillAyd]

Looks pretty good - general comments

[WillAyd]

Can you use is_list_like from pandas._libs.lib here instead? I think should help simplify the logic

[guipleite]

Not sure if it will simplify it because is_list_like returns True to both dictionaries and lists, however we only want it to enter in this statement if formatters is a list.

[WillAyd]

I think this can be simplified if you just precede this condition with:

if formatters is None:
    formatters = {}

And then go through the conditions. Right now there is a lot of duplication of conditions which makes it tougher to reason about

[gabriellm1]

Made an adjustment, is it better now?

[simonjayhawkins]

@WillAyd

"You can use a protocol class with isinstance() ..." "isinstance() also works with the predefined protocols in typing such as Iterable." https://mypy.readthedocs.io/en/latest/protocols.html#using-isinstance-with-protocols

We could therefore use isinstance checks that match the types added to the function signatures.

in this case the logic could be as simple as..

        if isinstance(formatters, Sequence) and len(frame.columns) != len(formatters):
            msg = (
                "Formatters length({flen}) should match"
                " DataFrame number of columns({dlen})"
            ).format(flen=len(formatters), dlen=len(frame.columns))
            raise ValueError(msg)
        self.formatters = formatters if formatters is not None else {}

thoughts?

[gabriellm1]

Looks good for me and it works!

[simonjayhawkins]

we don't use this pattern in the codebase, so don't make these changes in this PR. it's a discussion point going forward.

[simonjayhawkins]

also, although mentioned previously, formatters type is currently Union[List[Callable], Tuple[Callable, ...], Dict[Union[str, int], Callable] since the code uses isinstance with list, tuple and dict for flow control.

going forward, formatters type could be as permissive as Union[Sequence[Callable], Mapping[Union[str, int], Callable]

and then the protocol based isinstance checks would be more applicable.

[WillAyd]

I think this can be simplified if you just precede this condition with:

if formatters is None:
    formatters = {}

And then go through the conditions. Right now there is a lot of duplication of conditions which makes it tougher to reason about

[jreback]

this logic is quite complicated, can you not do

if formaters is not None and not do_len_comparision:
     raise....
self.formatters = formatters or {}

?

[gabriellm1]

Is it better now?

[simonjayhawkins]

lgtm. @WillAyd @jreback

[WillAyd]

Thanks @guipleite !