Skip to content

Remove Unnecessary Subclasses from test_excel #26553

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 4 commits into from
May 30, 2019
Merged

Remove Unnecessary Subclasses from test_excel #26553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ecf536a
Removed unnecessary writer subclassing
WillAyd May 29, 2019
2fcb2b1
Fixed issue in class creation of ExcelWriter
WillAyd May 29, 2019
a26b6c0
Removed unnecessary merge_cells
WillAyd May 29, 2019
39e4d4e
Removed sparsely used parametrization
WillAyd May 29, 2019
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
2 changes: 1 addition & 1 deletion pandas/io/excel/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ class ExcelWriter(metaclass=abc.ABCMeta):
def __new__(cls, path, engine=None, **kwargs):
# only switch class if generic(ExcelWriter)

if issubclass(cls, ExcelWriter):
if cls is ExcelWriter:
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't a user-facing bug but trying to import any of the ExcelWriter subclasses would exhibit rather surprising behavior which causes a test failure when getting rid of the subclassing. For instance:

>>> from pandas.io.excel._openpyxl import _OpenpyxlWriter
>>> writer = _OpenpyxlWriter('foo.xlsx')
>>> type(writer)
<class 'pandas.io.excel._xlsxwriter._XlsxWriter'>

From the comment directly preceding this it appears this condition was backwards, so changed as such

if engine is None or (isinstance(engine, str) and
engine == 'auto'):
if isinstance(path, str):
Expand Down
42 changes: 19 additions & 23 deletions pandas/tests/io/test_excel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2065,11 +2065,10 @@ def test_path_local_path(self, merge_cells, engine, ext):


@td.skip_if_no('openpyxl')
@pytest.mark.parametrize("merge_cells,ext,engine", [
(None, '.xlsx', 'openpyxl')])
class TestOpenpyxlTests(_WriterBase):
@pytest.mark.parametrize("ext", ['.xlsx'])
Copy link
Member Author

Choose a reason for hiding this comment

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

Only one test in each of these classes was actually using the engine parameter so figured cleaner to just specify there

Copy link
Member Author

Choose a reason for hiding this comment

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

merge_cells was also never used but rather required due to the mix of pytest / subclassing paradigms; I've done away with it entirely here

class TestOpenpyxlTests:

def test_to_excel_styleconverter(self, merge_cells, ext, engine):
def test_to_excel_styleconverter(self, ext):
from openpyxl import styles

hstyle = {
Expand Down Expand Up @@ -2123,7 +2122,7 @@ def test_to_excel_styleconverter(self, merge_cells, ext, engine):
assert kw['number_format'] == number_format
assert kw['protection'] == protection

def test_write_cells_merge_styled(self, merge_cells, ext, engine):
def test_write_cells_merge_styled(self, ext):
from pandas.io.formats.excel import ExcelCell

sheet_name = 'merge_styled'
Expand Down Expand Up @@ -2157,7 +2156,7 @@ def test_write_cells_merge_styled(self, merge_cells, ext, engine):

@pytest.mark.parametrize("mode,expected", [
('w', ['baz']), ('a', ['foo', 'bar', 'baz'])])
def test_write_append_mode(self, merge_cells, ext, engine, mode, expected):
def test_write_append_mode(self, ext, mode, expected):
import openpyxl
df = DataFrame([1], columns=['baz'])

Expand All @@ -2169,7 +2168,7 @@ def test_write_append_mode(self, merge_cells, ext, engine, mode, expected):
wb.worksheets[1]['A1'].value = 'bar'
wb.save(f)

writer = ExcelWriter(f, engine=engine, mode=mode)
writer = ExcelWriter(f, engine='openpyxl', mode=mode)
df.to_excel(writer, sheet_name='baz', index=False)
writer.save()

Expand All @@ -2182,12 +2181,11 @@ def test_write_append_mode(self, merge_cells, ext, engine, mode, expected):


@td.skip_if_no('xlwt')
@pytest.mark.parametrize("merge_cells,ext,engine", [
(None, '.xls', 'xlwt')])
class TestXlwtTests(_WriterBase):
@pytest.mark.parametrize("ext,", ['.xls'])
class TestXlwtTests:

def test_excel_raise_error_on_multiindex_columns_and_no_index(
self, merge_cells, ext, engine):
self, ext):
# MultiIndex as columns is not yet implemented 9794
cols = MultiIndex.from_tuples([('site', ''),
('2014', 'height'),
Expand All @@ -2197,16 +2195,15 @@ def test_excel_raise_error_on_multiindex_columns_and_no_index(
with ensure_clean(ext) as path:
df.to_excel(path, index=False)

def test_excel_multiindex_columns_and_index_true(self, merge_cells, ext,
engine):
def test_excel_multiindex_columns_and_index_true(self, ext):
cols = MultiIndex.from_tuples([('site', ''),
('2014', 'height'),
('2014', 'weight')])
df = pd.DataFrame(np.random.randn(10, 3), columns=cols)
with ensure_clean(ext) as path:
df.to_excel(path, index=True)

def test_excel_multiindex_index(self, merge_cells, ext, engine):
def test_excel_multiindex_index(self, ext):
# MultiIndex as index works so assert no error #9794
cols = MultiIndex.from_tuples([('site', ''),
('2014', 'height'),
Expand All @@ -2215,7 +2212,7 @@ def test_excel_multiindex_index(self, merge_cells, ext, engine):
with ensure_clean(ext) as path:
df.to_excel(path, index=False)

def test_to_excel_styleconverter(self, merge_cells, ext, engine):
def test_to_excel_styleconverter(self, ext):
import xlwt

hstyle = {"font": {"bold": True},
Expand All @@ -2234,21 +2231,20 @@ def test_to_excel_styleconverter(self, merge_cells, ext, engine):
assert xlwt.Alignment.HORZ_CENTER == xls_style.alignment.horz
assert xlwt.Alignment.VERT_TOP == xls_style.alignment.vert

def test_write_append_mode_raises(self, merge_cells, ext, engine):
def test_write_append_mode_raises(self, ext):
msg = "Append mode is not supported with xlwt!"

with ensure_clean(ext) as f:
with pytest.raises(ValueError, match=msg):
ExcelWriter(f, engine=engine, mode='a')
ExcelWriter(f, engine='xlwt', mode='a')


@td.skip_if_no('xlsxwriter')
@pytest.mark.parametrize("merge_cells,ext,engine", [
(None, '.xlsx', 'xlsxwriter')])
class TestXlsxWriterTests(_WriterBase):
@pytest.mark.parametrize("ext", ['.xlsx'])
class TestXlsxWriterTests:

@td.skip_if_no('openpyxl')
def test_column_format(self, merge_cells, ext, engine):
def test_column_format(self, ext):
# Test that column formats are applied to cells. Test for issue #9167.
# Applicable to xlsxwriter only.
with warnings.catch_warnings():
Expand Down Expand Up @@ -2292,12 +2288,12 @@ def test_column_format(self, merge_cells, ext, engine):

assert read_num_format == num_format

def test_write_append_mode_raises(self, merge_cells, ext, engine):
def test_write_append_mode_raises(self, ext):
msg = "Append mode is not supported with xlsxwriter!"

with ensure_clean(ext) as f:
with pytest.raises(ValueError, match=msg):
ExcelWriter(f, engine=engine, mode='a')
ExcelWriter(f, engine='xlsxwriter', mode='a')


class TestExcelWriterEngineTests:
Expand Down