TYP: typing errors in _xlsxwriter.py #35994

[fangchenli]

• closes TYP: type error in pandas/io/excel/_xlsxwriter.py #35994
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff

[simonjayhawkins]

Thanks @VirosaLi for the PR

[simonjayhawkins]

why is a variable type annotation needed here?

[fangchenli]

mypy would see it as None without annotation.

pandas/io/excel/_xlsxwriter.py:196: error: "None" has no attribute "close"  [attr-defined]
pandas/io/excel/_xlsxwriter.py:207: error: "None" has no attribute "add_worksheet"  [attr-defined]
pandas/io/excel/_xlsxwriter.py:225: error: "None" has no attribute "add_format"  [attr-defined]

[simonjayhawkins]

we have ignore_missing_imports=True in setup.cfg, so unresolved imports should resolve to Any

see https://mypy.readthedocs.io/en/stable/running_mypy.html#running-mypy-and-managing-imports

pandas\io\excel_xlsxwriter.py:190: note: Revealed type is 'Any'

do you know how mypy is picking it up as None in your setup.

[simonjayhawkins]

I've got a branch going to try and fix these errors, see https://github.com/pandas-dev/pandas/compare/master...simonjayhawkins:typing?expand=1

There I currently got assert self.book is not None in def save(self) and def write_cells but I'm not convinced that should be necessary either, but I can't find a similar issue in mypy tracker.

I think the issue could also be on our side...

in class ExcelWriter we have

   # declare external properties you can count on
    book = None

which not only seems redundant but you can't really count on attributes that don't have the relevant protocol, in this case close, add_worksheet, and add_format, so the intention seems misguided.

so we have a few options here

1. remove book=None from ExcelWriter
2. add assert self.book is not None in functions to ensure self.book is initialised
3. find/log mypy issue and add a type ignore
4. add variable type annotation to self.book in __init__ (as you've done here)

IMO
(1) may be best
(2) is the recommended practice for dealing with attributes that may be uninitialised (but I think that is not 100% relevant here since the attribute should always be initialised in __init__)

from https://mypy.readthedocs.io/en/stable/kinds_of_types.html?highlight=assert#optional-types-and-the-none-type

Sometimes mypy doesn’t realize that a value is never None. This notably happens when a class instance can exist in a partially defined state, where some attribute is initialized to None during object construction, but a method assumes that the attribute is no longer None. Mypy will complain about the possible None value. You can use assert x is not None to work around this in the method:

(3) ???
(4) personally I'm not keen on redundant (or shouldn't be necessary) variable annotations, they are harder to track and identify when no longer needed (unlike casts and ignores that we have checks for). AFAICT they can also be used to lie to the type checker when the assignment should be Any, see #29677 (comment).

wdyt?

[fangchenli]

I think (1) is the best. Adding variable type annotation here does seem odd.

[simonjayhawkins]

Thanks @VirosaLi lgtm. failure unrelated

=========================== short test summary info ============================
FAILED pandas/tests/util/test_show_versions.py::test_show_versions - UserWarn...
= 1 failed, 70474 passed, 4062 skipped, 1023 xfailed, 6 xpassed in 557.07s (0:09:17) =

[mroeschke]

Thanks @VirosaLi