Remove Unnecessary Subclasses from test_excel #26553
Conversation
WillAyd
added
Testing
| @pytest.mark.parametrize("merge_cells,ext,engine", [ | ||
| (None, '.xlsx', 'openpyxl')]) | ||
| class TestOpenpyxlTests(_WriterBase): | ||
| @pytest.mark.parametrize("ext", ['.xlsx']) |
There was a problem hiding this comment.
Only one test in each of these classes was actually using the engine parameter so figured cleaner to just specify there
There was a problem hiding this comment.
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
| @@ -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: | |||
There was a problem hiding this comment.
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
Codecov Report
@@ Coverage Diff @@
## master #26553 +/- ##
==========================================
- Coverage 91.77% 91.76% -0.01%
==========================================
Files 174 174
Lines 50649 50649
==========================================
- Hits 46483 46479 -4
- Misses 4166 4170 +4
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #26553 +/- ##
==========================================
- Coverage 91.77% 91.76% -0.01%
==========================================
Files 174 174
Lines 50649 50649
==========================================
- Hits 46483 46479 -4
- Misses 4166 4170 +4
Continue to review full report at Codecov.
|
|
@WillAyd you have the other excel test PR; this lgtm. so merge if going to be a conflict (or rebase this after) |
More cleanup...