Conversation
|
Hello @stevenbw! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on January 20, 2019 at 11:34 Hours UTC |
Codecov Report
@@ Coverage Diff @@
## master #24749 +/- ##
=======================================
Coverage 92.38% 92.38%
=======================================
Files 166 166
Lines 52358 52358
=======================================
Hits 48373 48373
Misses 3985 3985
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24749 +/- ##
==========================================
+ Coverage 92.38% 92.39% +<.01%
==========================================
Files 166 166
Lines 52358 52358
==========================================
+ Hits 48373 48375 +2
+ Misses 3985 3983 -2
Continue to review full report at Codecov.
|
|
@stevenbw The files will need to be named test_base.py and test_xlrd.py |
| @@ -128,96 +77,6 @@ def set_engine(self, request): | |||
| yield | |||
| setattr(self, func_name, old_func) | |||
|
|
|||
| @td.skip_if_no("xlrd", "1.0.1") # see gh-22682 | |||
| def test_usecols_int(self, ext): | |||
There was a problem hiding this comment.
you'll probably just want to split by complete classes in this PR and leave refactoring the classes to a follow-on PR.
pandas/tests/io/excel/test_xlrd.py
Outdated
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def ignore_xlrd_time_clock_warning(): |
There was a problem hiding this comment.
any code required by both test_base.py and test_xlrd.py should probably be in test_base.py
pandas/tests/io/excel/test_xlrd.py
Outdated
| class XlrdReadingTestsBase(XlrdSharedItems): | ||
| # This is based on ExcelWriterBase | ||
|
|
||
| @pytest.fixture(autouse=True, params=['xlrd']) |
There was a problem hiding this comment.
code duplicated from test_base
pandas/tests/io/excel/test_base.py
Outdated
| # This is based on ExcelWriterBase | ||
|
|
||
| @pytest.fixture(autouse=True, params=['xlrd', None]) | ||
| @pytest.fixture(autouse=True, params=[None]) |
There was a problem hiding this comment.
maybe a better name for test_base would be test_common or test_all_engines. this split is creating too much code duplication. maybe a split on reader/writer tests first before splitting out xlrd
|
Agreed high level with @simonjayhawkins think there are just too many things moved to Did this yield the same number of tests before and after? |
jreback
left a comment
There was a problem hiding this comment.
so before this is actually split; it needs to be fixturized a bit more I think. not really sure the best way of doing this, but if you are just moving files, then that can be in a single PR. We don't want to mix moving things with changing things as its very hard to tell what is going on. So first move w/o any changes, then new PR' to change.
|
Thanks for the support @simonjayhawkins, @WillAyd, @jreback :). I have taken on board your comments and attempted to simply split the file without separating classes or duplicating code. However, due to classes relying on previous classes, for example Unless I am missing something, it seems the ideal solution would be to rewrite the classes so they can be decoupled. I appreciate this would be a big change though. |
|
@stevenbw the biggest classes by far are |
|
@simonjayhawkins Yes they could. They both have a fundamental dependency on |
|
Can you merge master here? Also we just refactored the excel code in #25153 so you may be able to mimic that set up on the test side |
|
Yes I will take a look, should be able to mimic what you have done. |
Split tests/io/test_excel.py into two files, base.py and xlrd.py located in tests/io/excel/. The previous file was very large and a logical split has been applied to separate the
xlrdtests.git diff upstream/master -u -- "*.py" | flake8 --diff