BUG: format multiple CSS selectors correctly

[attack68]

• closes BUG: using a list in "selector" for set_table_styles produces wrong rules #34061
• tests added / passed
• Ensure all linting tests pass, see here for how to run them
• whatsnew entry

Fixes a bug that doesn't format multiple CSS elements in selectors properly.

[attack68]

Anyone know how to correctly define a TypedDict for testing??

from typing import TypedDict failed on all python 3.7.

from typing_extensions import TypedDict failed on some python 3.7 builds since there is no typing_extensions module in some of them??

[jreback]

you mean like Dict[str, Any] or whatever?

[attack68]

you mean like Dict[str, Any] or whatever?

to get this function to work nicely with mypy I needed to add:

class CSSDict(TypedDict):
    selector: str
    props: CSSProperties

which needed either

from typing import TypedDict  # 3.8+
from typing_extensions import TypedDict  # only included in some of the 3.7- builds.

[jreback]

can u make a data class instead?

[jreback]

@simonjayhawkins any suggestions here

[attack68]

OK I reformatted and ignored the structure, just used cast instead to fix mypy.

@jreback this is greenish now for review

[simonjayhawkins]

just used cast instead to fix mypy.

we should only use a cast if know better than mypy or it is a false positive.

can you add a comment in the code, about why we can guarantee that style["selector"] is a str and not a CSSList if styles is of type CSSStyles which is List[Dict[str, CSSProperties]] and CSSProperties is of type Union[str, CSSList]

the list comprehension has 4 for statements. If this is refactored, does that help mypy?

[attack68]

@simonjayhawkins

Thats why I tried to incorporate the TypedDict to make it clearer previously:

class CSSDict(TypedDict):
    selector: str
    props: CSSProperties

But didn't work for some of the 3.7builds which didn't have typing_extensions

so I reverted to

Dict[str, Union[str, CSSProperties]]

But I know that in the case of the selector key it is only str type, so I think cast here is OK?

[simonjayhawkins]

Thats why I tried to incorporate the TypedDict to make it clearer previously:

class CSSDict(TypedDict):
    selector: str
    props: CSSProperties

in the code we have the aliases...

CSSPair = Tuple[str, Union[str, int, float]]
CSSList = List[CSSPair]
CSSProperties = Union[str, CSSList]
CSSStyles = List[Dict[str, CSSProperties]]

But I know that in the case of the selector key it is only str type, so I think cast here is OK?

so without TypedDict that knowledge about CSSDict is not codified into the type aliases, so a comment in the code to help future readers may be beneficial.

from https://github.com/microsoft/pyright/blob/master/docs/typed-libraries.md

Typed Dictionaries, Data Classes, and Named Tuples
If your library runs only on newer versions of Python, you are encouraged to use some of the new type-friendly classes.

NamedTuple (described in PEP 484) is preferred over namedtuple.

Data classes (described in PEP 557) is preferred over untyped dictionaries.

TypedDict (described in PEP 589) is preferred over untyped dictionaries.

so if we can't use TypedDict we should use data classes as suggested by @jreback in #39942 (comment), or less preferable convert the untyped dict to a namedtuple.

[attack68]

@simonjayhawkins I investigated the DataClass solution which was new to me, and correct me if I'm wrong, but since the api input args use dicts and the user cannot be expected to pass in a DataClass then there would have to be a lot of internal mechanics to coordinate this, let alone changing hundreds of lines of tests (which I already did once and don't fancy doing again). Primarily since d['selector'] doesn't work for a dataclass: you have to change to d.selector. So I think thats why I disregarded that one.

And then I considered the named tuple, and again from memory I think this faced exactly the same issue.

I'm tempted to fix this very trivially, in the future, with the TypedDict when it becomes fully available in pandas!

I understand he point about the aliases that exist in the code and helping future readers, but two months ago there wasn't any typing for these and a mismatch of patterns across the module (but it is improving) , so I'm keen just to code comment it for now. :)

[simonjayhawkins]

but since the api input args use dicts and the user cannot be expected to pass in a DataClass

if that's the case, then indeed we can't change that.

I understand he point about the aliases that exist in the code and helping future readers, but two months ago there wasn't any typing for these and a mismatch of patterns across the module (but it is improving) , so I'm keen just to code comment it for now. :)

code comment is fine

[jreback]

looks fine. ping on green.

[attack68]

@jreback ping all green.

[jreback]

thanks @attack68