Skip to content

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

Merged
merged 2 commits into from
Jan 2, 2020

Conversation

ShaharNaveh
Copy link
Member

@ShaharNaveh ShaharNaveh commented Dec 22, 2019

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@pep8speaks
Copy link

pep8speaks commented Dec 22, 2019

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

@ShaharNaveh ShaharNaveh marked this pull request as ready for review December 22, 2019 12:50
@gfyoung gfyoung added the Typing type annotations, mypy/pyright type checking label Dec 22, 2019
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]:
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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

@ShaharNaveh ShaharNaveh requested a review from WillAyd December 25, 2019 22:38
Copy link
Member

@WillAyd WillAyd left a 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

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]:
Copy link
Member

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

"""
# 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]:
Copy link
Member

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

@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

@ShaharNaveh ShaharNaveh requested a review from WillAyd December 29, 2019 11:24
@ShaharNaveh ShaharNaveh force-pushed the TYP-io-formats-excel branch 4 times, most recently from 8e4b2e6 to 685e455 Compare December 29, 2019 14:59
@jreback jreback added this to the 1.0 milestone Jan 1, 2020
@jreback
Copy link
Contributor

jreback commented Jan 1, 2020

lgtm. over to you @WillAyd

@WillAyd WillAyd merged commit 9f97d11 into pandas-dev:master Jan 2, 2020
@WillAyd
Copy link
Member

WillAyd commented Jan 2, 2020

Thanks @MomIsBestFriend for this and all the others

@ShaharNaveh ShaharNaveh deleted the TYP-io-formats-excel branch January 2, 2020 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants