Split Excel IO Into Sub-Directory
State: closed

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

Labels:
name: Refactor
description: Internal refactoring of code
name: IO Excel
description: read_excel, to_excel

Comments:
Hello @WillAyd! Thanks for updating the PR.















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

##### Comment last updated on February 11, 2019 at 00:59 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=h1) Report
> Merging [#25153](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/e3b095074c5c3b29ec25a994cd204349b872d961?src=pr&el=desc) will **decrease** coverage by `49.76%`.
> The diff coverage is `22.59%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25153/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=tree)

```diff
@@             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%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/excel/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL19faW5pdF9fLnB5) | `100% <100%> (ø)` | |
| [pandas/io/excel/\_openpyxl.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL19vcGVucHl4bC5weQ==) | `12.1% <12.1%> (ø)` | |
| [pandas/io/excel/\_xlsxwriter.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL194bHN4d3JpdGVyLnB5) | `19.44% <19.44%> (ø)` | |
| [pandas/io/excel/\_xlwt.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL194bHd0LnB5) | `19.69% <19.69%> (ø)` | |
| [pandas/io/excel/\_xlrd.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL194bHJkLnB5) | `20.89% <20.89%> (ø)` | |
| [pandas/io/excel/\_util.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL191dGlsLnB5) | `26.88% <26.88%> (ø)` | |
| [pandas/io/excel/\_base.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL19iYXNlLnB5) | `27.69% <27.69%> (ø)` | |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| ... and [137 more](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=footer). Last update [e3b0950...323019e](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=h1) Report
> Merging [#25153](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/5f73594d49ea06ada4c6779006c00aaf41935595?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `90.11%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25153/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=tree)

```diff
@@            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%)` | :arrow_down: |
| #single | `41.89% <22.71%> (-0.25%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/excel/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL19faW5pdF9fLnB5) | `100% <100%> (ø)` | |
| [pandas/io/excel/\_util.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL191dGlsLnB5) | `80.64% <80.64%> (ø)` | |
| [pandas/io/excel/\_openpyxl.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL19vcGVucHl4bC5weQ==) | `84.71% <84.71%> (ø)` | |
| [pandas/io/excel/\_base.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL19iYXNlLnB5) | `92.52% <92.52%> (ø)` | |
| [pandas/io/excel/\_xlrd.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL194bHJkLnB5) | `94.02% <94.02%> (ø)` | |
| [pandas/io/excel/\_xlsxwriter.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL194bHN4d3JpdGVyLnB5) | `94.44% <94.44%> (ø)` | |
| [pandas/io/excel/\_xlwt.py](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL194bHd0LnB5) | `98.48% <98.48%> (ø)` | |
| ... and [4 more](https://codecov.io/gh/pandas-dev/pandas/pull/25153/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=footer). Last update [5f73594...85f2fe8](https://codecov.io/gh/pandas-dev/pandas/pull/25153?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

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.
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 <notifications@github.com> 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.

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
```
> 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 

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.
@WillAyd yep agree about readers/writers. do we have an issue for this split (or just the comment)?
lgtm. @TomAugspurger 
No issue just comment. Happy to open one
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 <notifications@github.com> wrote:
> 
> @gfyoung commented on this pull request.
> 
> In pandas/io/excel/__init__.py <https://github.com/pandas-dev/pandas/pull/25153#discussion_r254518269>:
> 
> > @@ -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 <https://github.com/pandas-dev/pandas/pull/25153#discussion_r254518269>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlOUVXw1_SZQqiKXJrmOnVUkfxmOHGFks5vK4CpgaJpZM4aih6T>.
> 


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
> 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.
@WillAyd, any chance you can resolve the conflicts and get this merged so I can continue working on #25092? 
thanks @WillAyd