read_excel and Excel File Engine conflict #26736
Conversation
Codecov Report
@@ Coverage Diff @@
## master #26736 +/- ##
==========================================
- Coverage 91.78% 91.77% -0.01%
==========================================
Files 174 174
Lines 50703 50706 +3
==========================================
- Hits 46538 46537 -1
- Misses 4165 4169 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26736 +/- ##
==========================================
- Coverage 91.71% 91.71% -0.01%
==========================================
Files 178 178
Lines 50771 50774 +3
==========================================
Hits 46567 46567
- Misses 4204 4207 +3
Continue to review full report at Codecov.
|
simonjayhawkins
added
Error Reporting
|
|
||
| if not isinstance(io, ExcelFile): | ||
| io = ExcelFile(io, engine=engine) | ||
| elif engine and engine != io.engine: |
There was a problem hiding this comment.
Is io.engine ever None? Coverage here might be a little suspect for cases like:
read_excel(ExcelFile(), engine='xlrd')which I suppose should work fine (assuming xlrd is default) but may not be covered here explicitly
There was a problem hiding this comment.
Current behaviour is io.engine in this line will default to 'xlrd' since that's in the ExcelFile constuctor -> added a test case to make this more explicit.
|
FYI not necessarily going to cause merge conflicts I think but I also just opened #26740. Depending on which merges first we may need to move the test added here around |
pandas/tests/io/test_excel.py
Outdated
| expected = pd.Series([1, 2, 3], name='a') | ||
| tm.assert_series_equal(actual, expected) | ||
|
|
||
| def test_read_excel_engine_value(self, ext): |
There was a problem hiding this comment.
can you parameterize with (for the ExcelFile), engine=['xlrd', 'openpyxl']
should fail for both
There was a problem hiding this comment.
Just to confirm my current implementation doesn't throw for the following case:
e = ExcelFile("blah.any", engine='openpyxl')
pd.read_excel(e, engine='openpyxl')
As around 15 of the test cases in test_excel used this construction - if you think the above should throw them I am happy to update the test cases - ( its just a simple update to the cd_and_set_engine fixture )
|
Can you merge master? Just changed around the structure of the tests I think you should move the new test to the one specific to TestExcelFileRead |
|
Thanks @WillAyd -the new structure is great btw really splits up the excel tests nicely! |
|
thanks @alimcmaster1 |
4 participants
git diff upstream/master -u -- "*.py" | flake8 --diffcc. @WillAyd
Note I only decided to raise an error when the engine specified to
read_excelwas different to that of the ExcelFile - otherwise around 15 test cases would need to be changed - let me know what you think