Repository: pandas-dev/pandas
Pull request: TYP: typing errors in _xlsxwriter.py #35994
- [x] closes #35994
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`



Problem statement:
TYP: type error in `pandas/io/excel/_xlsxwriter.py`
Those are some of the typing errors generated by mypy after unpining it.

```
pandas/io/excel/_xlsxwriter.py:105: error: "object" has no attribute "__iter__"; maybe "__str__" or "__dir__"? (not iterable)  [attr-defined]
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]
```

Hints from issue discussion:

Commit message: Merge remote-tracking branch 'upstream/master' into type-xlsxwriter

Patch to review:
diff --git a/pandas/io/excel/_xlsxwriter.py b/pandas/io/excel/_xlsxwriter.py
index 85a1bb031f457..7bbc7a9b513a4 100644
--- a/pandas/io/excel/_xlsxwriter.py
+++ b/pandas/io/excel/_xlsxwriter.py
@@ -1,3 +1,5 @@
+from typing import Dict
+
 import pandas._libs.json as json
 
 from pandas.io.excel._base import ExcelWriter
@@ -8,7 +10,7 @@ class _XlsxStyler:
     # Map from openpyxl-oriented styles to flatter xlsxwriter representation
     # Ordering necessary for both determinism and because some are keyed by
     # prefixes of others.
-    STYLE_MAPPING = {
+    STYLE_MAPPING: Dict[str, list] = {
         "font": [
             (("name",), "font_name"),
             (("sz",), "font_size"),
@@ -170,7 +172,7 @@ def __init__(
         **engine_kwargs,
     ):
         # Use the xlsxwriter module as the Excel writer.
-        import xlsxwriter
+        from xlsxwriter import Workbook
 
         if mode == "a":
             raise ValueError("Append mode is not supported with xlsxwriter!")
@@ -184,7 +186,7 @@ def __init__(
             **engine_kwargs,
         )
 
-        self.book = xlsxwriter.Workbook(path, **engine_kwargs)
+        self.book: Workbook = Workbook(path, **engine_kwargs)
 
     def save(self):
         """


Code review comments:
diff_hunk: @@ -184,7 +186,7 @@ def __init__(
             **engine_kwargs,
         )
 
-        self.book = xlsxwriter.Workbook(path, **engine_kwargs)
+        self.book: Workbook = Workbook(path, **engine_kwargs)
path: pandas/io/excel/_xlsxwriter.py
text: @user1:
why is a variable type annotation needed here?

@author:
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]
```

@user1:
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.

@user1:
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 https://github.com/pandas-dev/pandas/pull/29677#discussion_r347171686. 


wdyt?


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

diff_hunk: @@ -8,7 +10,7 @@ class _XlsxStyler:
     # Map from openpyxl-oriented styles to flatter xlsxwriter representation
     # Ordering necessary for both determinism and because some are keyed by
     # prefixes of others.
-    STYLE_MAPPING = {
+    STYLE_MAPPING: Dict[str, list] = {
path: pandas/io/excel/_xlsxwriter.py
text: @user1:
list -> List and add type parameters.

I think `Dict[str, List[Tuple[Tuple[str, ...], str]]]`