Skip to content

TYP use typeddict to define cssdict #40947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
simonjayhawkins merged 6 commits into from
Apr 27, 2021
Merged

TYP use typeddict to define cssdict #40947

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8166430
use typeddict to define cssdict
MarcoGorelli Apr 14, 2021
3c98fcb
Merge remote-tracking branch 'upstream/master' into cssdict
MarcoGorelli Apr 15, 2021
af5b3b8
Merge remote-tracking branch 'upstream/master' into cssdict
MarcoGorelli Apr 25, 2021
1d5c6c1
wip
MarcoGorelli Apr 25, 2021
ab033a0
clean up
MarcoGorelli Apr 25, 2021
b6aa69f
dont construct cssdict
MarcoGorelli Apr 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion pandas/_typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@
# and use a string literal forward reference to it in subsequent types
# https://mypy.readthedocs.io/en/latest/common_issues.html#import-cycles
if TYPE_CHECKING:
from typing import final
from typing import (
TypedDict,
final,
)

from pandas._libs import (
Period,
Expand Down Expand Up @@ -70,6 +73,8 @@
else:
# typing.final does not exist until py38
final = lambda x: x
# typing.TypedDict does not exist until py38
TypedDict = dict
Copy link
Member

@simonjayhawkins simonjayhawkins Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't need to use the TypedDict at runtime, then we would not need this and we could only use TypedDict in TYPE_CHECKING blocks.

I see the downside being that aliases (and imports of aliases) would also need to be defined in the TYPE_CHECKING block and that we would not be able to construct the TypedDict explicitly in code. but that could be an upside.

no strong preference here yet.

Copy link
Member

@simonjayhawkins simonjayhawkins Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking some more. leave this for now.

from NEP 29, Dec 26, 2021 drop support for Python 3.7

so we should be able to drop python 3.7 on master after the 1.3 release.



# array-like
Expand Down
24 changes: 15 additions & 9 deletions pandas/io/formats/style_render.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
Sequence,
Tuple,
Union,
cast,
)
from uuid import uuid4

Expand All @@ -21,7 +20,10 @@
from pandas._config import get_option

from pandas._libs import lib
from pandas._typing import FrameOrSeriesUnion
from pandas._typing import (
FrameOrSeriesUnion,
TypedDict,
)
from pandas.compat._optional import import_optional_dependency

from pandas.core.dtypes.generic import ABCSeries
Expand All @@ -45,10 +47,14 @@
CSSPair = Tuple[str, Union[str, int, float]]
CSSList = List[CSSPair]
CSSProperties = Union[str, CSSList]
CSSStyles = List[Dict[str, CSSProperties]] # = List[CSSDict]
# class CSSDict(TypedDict): # available when TypedDict is valid in pandas
# selector: str
# props: CSSProperties


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


CSSStyles = List[CSSDict]


class StylerRenderer:
Expand Down Expand Up @@ -617,9 +623,9 @@ def _format_table_styles(styles: CSSStyles) -> CSSStyles:
return [
item
for sublist in [
[ # this is a CSSDict when TypedDict is available to avoid cast.
{"selector": x, "props": style["props"]}
for x in cast(str, style["selector"]).split(",")
[
CSSDict(selector=x, props=style["props"])
for x in style["selector"].split(",")
Copy link
Member

@simonjayhawkins simonjayhawkins Apr 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should need to create the CSSDict explicitly. mypy should check if the expression is a compatible type

    return [
        {"selector": selector, "props": css_dict["props"]}
        for css_dict in styles
        for selector in css_dict["selector"].split(",")
    ]

Copy link
Member Author

@MarcoGorelli MarcoGorelli Apr 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just tried that and got

pandas/io/formats/style_render.py:624: error: List comprehension has incompatible type List[Dict[str, Union[str, List[Tuple[str, Union[str, int, float]]]]]]; expected List[CSSDict]  [misc]

Copy link
Member

@simonjayhawkins simonjayhawkins Apr 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it maybe a mypy cache thing.

maybe better then to use for loops and append starting with css_dict : CSSDict = []

the list comp with 4 fors is too difficult to grok.

Copy link
Member

@simonjayhawkins simonjayhawkins Apr 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, seems ok

(pandas-dev) simon@P340:~/pandas (cssdict)$ mypy pandas
Success: no issues found in 1280 source files
(pandas-dev) simon@P340:~/pandas (cssdict)$ mypy pandas --no-incremental
Success: no issues found in 1280 source files
(pandas-dev) simon@P340:~/pandas (cssdict)$ mypy --version
mypy 0.812
(pandas-dev) simon@P340:~/pandas (cssdict)$

Copy link
Member

@simonjayhawkins simonjayhawkins Apr 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better then to use for loops and append starting with css_dict : CSSDict = []

should have been css_styles : CSSStyles = []

Copy link
Member Author

@MarcoGorelli MarcoGorelli Apr 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you're right!

Seems like

    ret = [
        {"selector": selector, "props": css_dict["props"]}
        for css_dict in styles
        for selector in css_dict["selector"].split(",")
    ]
    return ret

throws a mypy error, while

    return [
        {"selector": selector, "props": css_dict["props"]}
        for css_dict in styles
        for selector in css_dict["selector"].split(",")
    ]

doesn't - I wasn't expecting them to be different, and I had the former pattern, sorry about that

Copy link
Member

@simonjayhawkins simonjayhawkins Apr 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. mypy will infer the type of the intermediate variable instead of checking the return expression against the return type. could use...

ret: CSSStyles = ...
return ret

and mypy will check the list comp is compatible the variable type declaration

Copy link
Member Author

@MarcoGorelli MarcoGorelli Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for explaining/merging! will copy-and-paste next time

]
for style in styles
]
Expand Down