Split Excel IO Into Sub-Directory

[WillAyd]

Continued refactoring of Excel. There's a ton more that can be done here to clean up public / private naming conventions, but to keep diff minimal I've tried to just move things around. Exceptions are noted in comments

cc @tdamsma

[pep8speaks]

Hello @WillAyd! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 11, 2019 at 00:59 Hours UTC

[WillAyd]

Could obviously make this more succinct with a loop but again attempting to minimize diff

[gfyoung]

I'm not sure I follow - given that +/- 2000 line diff from git in this PR, what diff are you trying to minimize exactly?

[WillAyd]

Had to move this into the class as it would be a circular reference otherwise, though ideally this shouldn't be required at all in the base class

[WillAyd]

Here's the other part of the circular reference which I had to move to the class init to resolve

[codecov]

Codecov Report

Merging #25153 into master will decrease coverage by 49.76%.
The diff coverage is 22.59%.

@@             Coverage Diff             @@
##           master   #25153       +/-   ##
===========================================
- Coverage   92.37%   42.61%   -49.77%     
===========================================
  Files         166      173        +7     
  Lines       52408    53085      +677     
===========================================
- Hits        48412    22620    -25792     
- Misses       3996    30465    +26469

Flag	Coverage Δ
#multiple	?
#single	42.61% <22.59%> (-0.26%)	⬇️

Impacted Files	Coverage Δ
pandas/io/excel/__init__.py	100% <100%> (ø)
pandas/io/excel/_openpyxl.py	12.1% <12.1%> (ø)
pandas/io/excel/_xlsxwriter.py	19.44% <19.44%> (ø)
pandas/io/excel/_xlwt.py	19.69% <19.69%> (ø)
pandas/io/excel/_xlrd.py	20.89% <20.89%> (ø)
pandas/io/excel/_util.py	26.88% <26.88%> (ø)
pandas/io/excel/_base.py	27.69% <27.69%> (ø)
pandas/io/formats/latex.py	0% <0%> (-100%)	⬇️
pandas/core/categorical.py	0% <0%> (-100%)	⬇️
pandas/io/sas/sas_constants.py	0% <0%> (-100%)	⬇️
... and 137 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 e3b0950...323019e. Read the comment docs.

[codecov]

Codecov Report

Merging #25153 into master will decrease coverage by 0.02%.
The diff coverage is 90.11%.

@@            Coverage Diff             @@
##           master   #25153      +/-   ##
==========================================
- Coverage   91.93%   91.91%   -0.03%     
==========================================
  Files         166      173       +7     
  Lines       52187    52865     +678     
==========================================
+ Hits        47978    48589     +611     
- Misses       4209     4276      +67

Flag	Coverage Δ
#multiple	90.47% <90.11%> (-0.01%)	⬇️
#single	41.89% <22.71%> (-0.25%)	⬇️

Impacted Files	Coverage Δ
pandas/io/excel/__init__.py	100% <100%> (ø)
pandas/io/excel/_util.py	80.64% <80.64%> (ø)
pandas/io/excel/_openpyxl.py	84.71% <84.71%> (ø)
pandas/io/excel/_base.py	92.52% <92.52%> (ø)
pandas/io/excel/_xlrd.py	94.02% <94.02%> (ø)
pandas/io/excel/_xlsxwriter.py	94.44% <94.44%> (ø)
pandas/io/excel/_xlwt.py	98.48% <98.48%> (ø)
... and 4 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 5f73594...85f2fe8. Read the comment docs.

[tdamsma]

Would it be an idea to explicitly split the files in readers and writers? The openpyxl writer file is already pretty large by itself, and as I am working on a reader it will get even larger. Even the _base.py is prretty large, and there is very little shared code nor shared concern between the reader and writer functions.

[WillAyd]

Yea I thought about that too but I think the reader and writer modules would just be pretty large in that case. What other modules would you have think of besides those two?

…

Sent from my iPhone

On Feb 4, 2019, at 10:06 PM, Thijs Damsma ***@***.***> wrote: Would it be an idea to explicitly split the files in readers and writers? The openpyxl writer file is already pretty large by itself, and as I am working on a reader it will get even larger. Even the _base.py is prretty large, and there is very little shared code nor shared concern between the reader and writer functions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[tdamsma]

I think I wasn't clear, I mean split by library (openpyx, xlrd, whatever) as you propse, and additionally also split by read/write functionality. So that gives something like:

_base_reader.py
_base_writer.py
_openpyxl_reader.py
_openpyxl_writer.py
_xlrd_reader.py
_xlsxwriter_writer.py
etc

While I am writing this out this doesn't look so nice anymore. Other options:

• A reader and writer submodule in the excel module?
• Reverse the names like so:

_reader_base.py
_reader_openpyxl.py
_reader_xlrd.py
_writer_base.py
_writer_openpyxl.py
_writer_xlsxwriter.py
etc

[simonjayhawkins]

I mean split by library (openpyx, xlrd, whatever) as you propse, and additionally also split by read/write functionality. So that gives something like:

xref #24749

[WillAyd]

The only library that can both read and write is openpyxl, so splitting by reader/writer is superfluous for most libraries here. Not against doing it if that's the ultimate structure desired by everyone but at the moment I don't think that adds much value or would drastically minimize anything, save splitting _base and _openpyxl into two more modules.

[jreback]

@WillAyd yep agree about readers/writers. do we have an issue for this split (or just the comment)?

[jreback]

lgtm. @TomAugspurger

[WillAyd]

No issue just comment. Happy to open one

[gfyoung]

Why do we need to create this error variable?

[gfyoung]

Add a comment explaining what this maps. Perhaps reference _get_writer as well.

[gfyoung]

Hmm...I think we need some more docstrings.

[gfyoung]

Stray comment?

[WillAyd]

You bring up very valid questions but why would you want to tackle them in this PR? Really trying to limit this to pure copy / paste of code blocks so it doesn’t get out of hand just like we have done on almost any other similar change

…

On Feb 6, 2019, at 5:26 PM, gfyoung ***@***.***> wrote: @gfyoung commented on this pull request. In pandas/io/excel/__init__.py <#25153 (comment)>: > @@ -0,0 +1,16 @@ +from pandas.io.excel._base import read_excel, ExcelWriter, ExcelFile +from pandas.io.excel._openpyxl import _OpenpyxlWriter +from pandas.io.excel._util import register_writer +from pandas.io.excel._xlsxwriter import _XlsxWriter +from pandas.io.excel._xlwt import _XlwtWriter + +__all__ = ["read_excel", "ExcelWriter", "ExcelFile"] + + +register_writer(_OpenpyxlWriter) I'm not sure I follow - given that +/- 2000 line diff from git, what diff are you trying to minimize? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25153 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlOUVXw1_SZQqiKXJrmOnVUkfxmOHGFks5vK4CpgaJpZM4aih6T>.

[jreback]

can you merge master

big moves like this should have almost no changes to the actual code
as it’s very hard to see any
diffs

[gfyoung]

You bring up very valid questions but why would you want to tackle them in this PR? Really trying to limit this to pure copy / paste of code blocks so it doesn’t get out of hand just like we have done on almost any other similar change

@WillAyd : No need to get defensive here. 😉

I have no problems deferring this to another PR as follow-up.

[tdamsma]

@WillAyd, any chance you can resolve the conflicts and get this merged so I can continue working on #25092?

[jreback]

thanks @WillAyd