-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
TYP: Type hints in pandas/io/formats/excel.py #30400
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.
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
Conversation
Hello @MomIsBestFriend! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-12-29 14:59:34 UTC |
b4120e9
to
5b55597
Compare
5b55597
to
216f92e
Compare
pandas/io/formats/excel.py
Outdated
if inherited is not None: | ||
inherited = self.compute_css(inherited) | ||
|
||
self.inherited = inherited | ||
|
||
compute_css = CSSResolver() | ||
|
||
def __call__(self, declarations_str: str): | ||
def __call__(self, declarations_str: str) -> Dict[str, Dict]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This annotation as a return value would mean that this always needs to be a dict of dicts - is that true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
Because it's returning at the end
return self.build_xlstyle(properties)
and build_xlstyle
is returning a Dict
containing:
out = {
"alignment": self.build_alignment(props),
"border": self.build_border(props),
"fill": self.build_fill(props),
"font": self.build_font(props),
"number_format": self.build_number_format(props),
}
.........
return out
and each one of
[build_alignment, build_border, build_fill, build_font, build_number_format]
also returns a Dict
This is how I see it, you're way more experienced than I am, if you think otherwise I'd like to hear it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK I see makes sense. So can we go further and do Dict[str, Dict[str, str]]
to describe this exactly again? Subtypes are much more useful compared to just typing as Dict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat more tedious but we can add subtypes here without much extra effort so would prefer that in all cases
pandas/io/formats/excel.py
Outdated
if inherited is not None: | ||
inherited = self.compute_css(inherited) | ||
|
||
self.inherited = inherited | ||
|
||
compute_css = CSSResolver() | ||
|
||
def __call__(self, declarations_str: str): | ||
def __call__(self, declarations_str: str) -> Dict[str, Dict]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK I see makes sense. So can we go further and do Dict[str, Dict[str, str]]
to describe this exactly again? Subtypes are much more useful compared to just typing as Dict
pandas/io/formats/excel.py
Outdated
""" | ||
# TODO: memoize? | ||
properties = self.compute_css(declarations_str, self.inherited) | ||
return self.build_xlstyle(properties) | ||
|
||
def build_xlstyle(self, props): | ||
def build_xlstyle(self, props: Dict) -> Dict[str, Dict]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add subtypes for props? I think just Dict[str, str]
and can maybe annotate CSSResolver
as part of this
pandas/io/formats/excel.py
Outdated
@@ -95,8 +108,14 @@ def build_xlstyle(self, props): | |||
|
|||
# TODO: handle cell width and height: needs support in pandas.io.excel | |||
|
|||
def remove_none(d): | |||
"""Remove key where value is None, through nested dicts""" | |||
def remove_none(d: Dict[str, Any]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def remove_none(d: Dict[str, Any]) -> None: | |
def remove_none(d: Dict[str, Dict[str, str]]) -> None: |
Minor nit but this should just match what out
directly above is
216f92e
to
c693cff
Compare
8e4b2e6
to
685e455
Compare
lgtm. over to you @WillAyd |
Thanks @MomIsBestFriend for this and all the others |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff