Skip to content

TYP: typing errors in _xlsxwriter.py #35994 #35995

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.

Sign up for GitHub

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 6 commits into from
Aug 31, 2020
Merged

TYP: typing errors in _xlsxwriter.py #35994 #35995

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2b9d262
TYP: typing errors in _xlsxwriter.py #35994
fangchenli Aug 30, 2020
2b07bb4
Merge remote-tracking branch 'upstream/master' into type-xlsxwriter
fangchenli Aug 30, 2020
f21a189
TYP: add param type
fangchenli Aug 30, 2020
fdf141a
Merge remote-tracking branch 'upstream/master' into type-xlsxwriter
fangchenli Aug 31, 2020
5b5f5e0
TYP: remove book=None in base class
fangchenli Aug 31, 2020
5243e18
Merge remote-tracking branch 'upstream/master' into type-xlsxwriter
fangchenli Aug 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions pandas/io/excel/_xlsxwriter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from typing import Dict

import pandas._libs.json as json

from pandas.io.excel._base import ExcelWriter
Expand All @@ -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"),
Expand Down Expand Up @@ -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!")
Expand All @@ -184,7 +186,7 @@ def __init__(
**engine_kwargs,
)

self.book = xlsxwriter.Workbook(path, **engine_kwargs)
self.book: Workbook = Workbook(path, **engine_kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

why is a variable type annotation needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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]

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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


def save(self):
"""
Expand Down