CLN/TYP: pandas/io/formats/excel.py #37862

ivanovmg · 2020-11-15T15:30:45Z

xref TYP: investigate/fix ignored mypy errors #37715
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry
Handle two type ignore items in pandas/io/formats/excel.py
De-duplicate has_aliases variable by extracting property
Add typing for iterators yielding ExcelCell
Use named arguments for ExcelCell on some occasions (in two places de-duplicate by updating kwargs inplace)

simonjayhawkins

Thanks @ivanovmg for the PR. a couple of comments. will look in more detail shortly.

pandas/io/formats/excel.py

jbrockmendel · 2020-11-15T16:03:45Z

pandas/io/formats/excel.py

-                    # "Union[Sequence[Optional[Hashable]], bool]"; expected
-                    # "Sized"  [arg-type]
+            if self._has_aliases:
+                assert isinstance(self.header, Sized)


glad to see this. for some reason my local mypy diagrees with the CI about whether its L601 or L602 that has the problem

It seems that the problem was with len(self.header), so it should be line 602, but presumably one line was added on top since then.
EDIT: I guess I am not correct here...

once we add a runtime isinstance, this sort of duplicates the _has_aliases property.

since self.header is a sequence or bool, maybe _has_aliases could reflect that instead of the explicit checks for the 4 classes.

if we have a guarantee from the is_* call, a cast would be more appropriate than the assert.

Updated.
Used casting instead of asserting.
In _has_aliases check if not boolean rather than 4 sequence-like classes.
Is that what you meant?

If the types are correct for the header parameter of ExcelFormatter then this should be fine.

also cast is a no-op, so no harm using self.header = cast(Sequence, self.header) and the other self.header -> header changes wouldn't be needed.

OK, changed the casting as you suggested.

ivanovmg · 2020-11-16T14:48:15Z

It's green.

simonjayhawkins

Thanks @ivanovmg. some more comments (after thinking about this some more!)

pandas/io/formats/excel.py

simonjayhawkins · 2020-11-16T15:08:49Z

pandas/io/formats/excel.py

+    @property
+    def _has_aliases(self) -> bool:
+        """Whether the aliases for column names are present."""
+        return not isinstance(self.header, bool)


thinking some more, maybe best not to use bool.

we've never really had a discussion on use of bool in type annotations, but sometimes we are actually accepting any truthy value and other times we explicitly check if x is False or if x is True.

the header parameter of the docstring of ExcelFormatter should probably be updated to match. It currently is

header : boolean or list of string, default True Write out column names. If a list of string is given it is assumed to be aliases for the column names

even though (tuple, list, np.ndarray, ABCIndex) is the current accepted types on master.

and the current mypy issue is that header is annotated as Union[Sequence[Label], bool]

perhaps use is_list_like here. (ignoring that a str is a Sequence)

I replaced with is_list_like.
Also slightly updated docstring. Will it work?

there is still a discrepancy. Can a sequence of any hashable be passed or does it have to be a sequence of strings.

ExcelFormatter is called only from Styler.to_excel and NDFrame.to_excel.
According to these docstrings

header : bool or list of str, default True

I also looked through the test base and noticed that header is either bool or some sequence of strings (on some occasions).

I am not aware of other use cases, when header is a sequence of any hashables rather than strings.

ok can leave as is for now.

The docstrings of public methods document the public api, the type annotations should normally be as permissible as possible for argument positions and the docstring and type annotations should match.

we don't want to change the public api without discussion, so can circle back to this.

simonjayhawkins · 2020-11-16T15:11:10Z

pandas/io/formats/excel.py

+                        col=coloffset + i + 1,
+                        val=values[i],
+                        style=self.header_style,
+                        mergestart=lnum if spans[i] > 1 else None,


maybe spans[i] > 1 could first be assigned to another appropriately named variable to clarify intent and (slightly) reduce duplication.

I am a bit confused what spans[i] > 1 means. I guess it is whether there is a multi-level col. So, I used the variable is_multilevel. Please let me know what you think.

I think we are always multilevel in this function. maybe needs_merge_cells or spans_multiple_cells

These are good names indeed. Thank you!

jreback · 2020-11-17T01:33:46Z

lgtm. @simonjayhawkins generally these typing PRs will defer to you so merge when ready.

ivanovmg · 2020-11-22T21:09:41Z

@simonjayhawkins is there anything else expected from me on this PR?

pandas/io/formats/excel.py

Co-authored-by: Simon Hawkins <[email protected]>

jreback · 2020-11-24T13:38:12Z

thanks @ivanovmg

ivanovmg added 7 commits November 15, 2020 21:15

CLN: use enumerate

8d5d4bb

CLN: use kwargs for calling ExcelCell

b066f5f

CLN: de-duplicate by updating kwargs

3484f53

REF: extract property _has_aliases

5c2f160

TYP: fix typing error, remove ignore

ee884ca

TYP: type cell iterators

dab9e15

DOC: add docstring for _has_aliases

1ff8cb8

simonjayhawkins reviewed Nov 15, 2020

View reviewed changes

pandas/io/formats/excel.py Outdated Show resolved Hide resolved

pandas/io/formats/excel.py Outdated Show resolved Hide resolved

simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Nov 15, 2020

simonjayhawkins added this to the 1.2 milestone Nov 15, 2020

jbrockmendel reviewed Nov 15, 2020

View reviewed changes

ivanovmg added 5 commits November 15, 2020 23:07

CLN: remove unnecessary bool conversion

ee09d4d

REF: update handling merge* kwargs vs spans[i]

c5e1d9c

REF: avoid explicit header check for 4 isinstances

7d27a71

TYP: override self.header when casting

2f02811

Merge branch 'master' into cleanup/excel

a63b7d1

ivanovmg requested review from simonjayhawkins and jbrockmendel November 16, 2020 14:48

simonjayhawkins reviewed Nov 16, 2020

View reviewed changes

ivanovmg added 6 commits November 17, 2020 17:50

TYP: replace Iterator with Iterable

9fcfb32

REF: de-duplicate using local var is_multilevel

aa48fb6

REF: update _has_aliases using is_list_like

f4abe27

DOC: header accepts sequence of strings

f468397

CLN: rename is_multlevel -> spans_multiple_cells

4cf8f83

Merge branch 'master' into cleanup/excel

084e953

ivanovmg requested a review from simonjayhawkins November 22, 2020 21:10

simonjayhawkins reviewed Nov 23, 2020

View reviewed changes

pandas/io/formats/excel.py Outdated Show resolved Hide resolved

simonjayhawkins reviewed Nov 23, 2020

View reviewed changes

pandas/io/formats/excel.py Outdated Show resolved Hide resolved

ivanovmg and others added 3 commits November 24, 2020 18:08

REF: implement suggestion

8a9a40a

Co-authored-by: Simon Hawkins <[email protected]>

REF: implement suggestion

2ee661c

Co-authored-by: Simon Hawkins <[email protected]>

FIX: fix typo

c88aff9

jreback added the IO Excel read_excel, to_excel label Nov 24, 2020

jreback merged commit 7368b2a into pandas-dev:master Nov 24, 2020

ivanovmg deleted the cleanup/excel branch December 3, 2020 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN/TYP: pandas/io/formats/excel.py #37862

CLN/TYP: pandas/io/formats/excel.py #37862

ivanovmg commented Nov 15, 2020

simonjayhawkins left a comment

jbrockmendel Nov 15, 2020

ivanovmg Nov 15, 2020 •

edited

Loading

simonjayhawkins Nov 15, 2020

ivanovmg Nov 15, 2020

simonjayhawkins Nov 15, 2020

ivanovmg Nov 15, 2020

ivanovmg commented Nov 16, 2020

simonjayhawkins left a comment

simonjayhawkins Nov 16, 2020

ivanovmg Nov 17, 2020

simonjayhawkins Nov 17, 2020

ivanovmg Nov 17, 2020

simonjayhawkins Nov 23, 2020

simonjayhawkins Nov 16, 2020

ivanovmg Nov 17, 2020

simonjayhawkins Nov 17, 2020

ivanovmg Nov 17, 2020

jreback commented Nov 17, 2020

ivanovmg commented Nov 22, 2020

jreback commented Nov 24, 2020

CLN/TYP: pandas/io/formats/excel.py #37862

CLN/TYP: pandas/io/formats/excel.py #37862

Conversation

ivanovmg commented Nov 15, 2020

simonjayhawkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanovmg Nov 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanovmg commented Nov 16, 2020

simonjayhawkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 17, 2020

ivanovmg commented Nov 22, 2020

jreback commented Nov 24, 2020

ivanovmg Nov 15, 2020 •

edited

Loading