Skip to content
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.

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

Refactor init for Excel readers to _BaseExcelReader #26233

Merged

Conversation

tdamsma
Copy link
Contributor

@tdamsma tdamsma commented Apr 28, 2019

Pre-cursor for #25092. To support multiple excel readers the init function is moved to _BaseExcelReader. The function is made a bit more general to be able to support multiple excel readers.

  • tests passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@tdamsma tdamsma changed the title Excel read shared init to baseclass Refactor init for Excel readers to baseclass Apr 28, 2019
@tdamsma tdamsma changed the title Refactor init for Excel readers to baseclass Refactor init for Excel readers to _BaseExcelReader Apr 28, 2019
@jreback
Copy link
Contributor

jreback commented Apr 28, 2019

looks good. ping when ready. cc @WillAyd

@jreback jreback added the IO Excel read_excel, to_excel label Apr 28, 2019
try:
# GH 19779
filepath_or_buffer.seek(0)
except UnsupportedOperation:
Copy link
Member

Choose a reason for hiding this comment

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

I see this wasn't carried over - is it no longer valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now dealt with by wrapping url responses in BytesIO(urlopen(filepath_or_buffer).read()). So now all items with "read" can also support seek(0)

pandas/io/excel/_xlrd.py Outdated Show resolved Hide resolved
pandas/io/excel/_xlrd.py Outdated Show resolved Hide resolved
@tdamsma tdamsma marked this pull request as ready for review April 29, 2019 08:25
@tdamsma
Copy link
Contributor Author

tdamsma commented Apr 29, 2019

@jreback, @WillAyd, I don't understand the test failing tests, but other than that I think it's ready

@WillAyd
Copy link
Member

WillAyd commented Apr 29, 2019

@tdamsma restarted all of them. Not sure what is wrong with the checks_and_doc job off the top of my head but looks like this PR introduced some regressions with handling of S3 excel files as shown in the Travis failures.

If you could take a look would be great

@codecov
Copy link

codecov bot commented Apr 30, 2019

Codecov Report

Merging #26233 into master will decrease coverage by 0.01%.
The diff coverage is 90.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26233      +/-   ##
==========================================
- Coverage   91.97%   91.96%   -0.02%     
==========================================
  Files         175      175              
  Lines       52368    52376       +8     
==========================================
+ Hits        48164    48166       +2     
- Misses       4204     4210       +6
Flag Coverage Δ
#multiple 90.51% <90.32%> (-0.01%) ⬇️
#single 40.69% <29.03%> (-0.15%) ⬇️
Impacted Files Coverage Δ
pandas/io/excel/_xlrd.py 94.54% <100%> (+0.6%) ⬆️
pandas/io/excel/_base.py 92.13% <86.36%> (-0.72%) ⬇️
pandas/io/gbq.py 78.94% <0%> (-10.53%) ⬇️
pandas/core/frame.py 96.9% <0%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9feb3ad...903b188. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 30, 2019

Codecov Report

Merging #26233 into master will decrease coverage by 68.95%.
The diff coverage is 31.25%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #26233       +/-   ##
===========================================
- Coverage   91.97%   23.01%   -68.96%     
===========================================
  Files         175      175               
  Lines       52368    52377        +9     
===========================================
- Hits        48164    12055    -36109     
- Misses       4204    40322    +36118
Flag Coverage Δ
#multiple ?
#single 23.01% <31.25%> (-17.83%) ⬇️
Impacted Files Coverage Δ
pandas/io/excel/_xlrd.py 23.21% <30%> (-70.73%) ⬇️
pandas/io/excel/_base.py 27.94% <31.81%> (-64.91%) ⬇️
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/core/groupby/categorical.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-99.37%) ⬇️
pandas/core/apply.py 0% <0%> (-98.62%) ⬇️
pandas/io/formats/csvs.py 0% <0%> (-98.2%) ⬇️
... and 140 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9feb3ad...1edae5e. Read the comment docs.

@tdamsma tdamsma force-pushed the excel-read-shared-init-to-baseclass branch from 6e646e9 to 903b188 Compare April 30, 2019 08:27
@jreback jreback added this to the 0.25.0 milestone Apr 30, 2019
@jreback
Copy link
Contributor

jreback commented Apr 30, 2019

lgtm. @WillAyd

@tdamsma
Copy link
Contributor Author

tdamsma commented Apr 30, 2019

@WillAyd, I addressed the regression. Also I accidentally committed (and reverted) a setting to limit the test scope, that explains the codecov report.

@WillAyd WillAyd merged commit 7eff627 into pandas-dev:master Apr 30, 2019
@WillAyd
Copy link
Member

WillAyd commented Apr 30, 2019

Nice job @tdamsma - looking forward to the follow ups!

@tdamsma
Copy link
Contributor Author

tdamsma commented Apr 30, 2019

Great, thanks for the reviews

@tdamsma tdamsma deleted the excel-read-shared-init-to-baseclass branch May 1, 2019 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO Excel read_excel, to_excel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants