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

ENH: add if_sheet_exists='overlay' to ExcelWriter #42222

Merged
merged 14 commits into from Nov 17, 2021

Conversation

feefladder
Copy link
Contributor

@feefladder feefladder commented Jun 25, 2021

I mainly wanted this functionality myself, and am very open to input :)

feefladder pushed a commit to feefladder/pandas that referenced this pull request Jun 26, 2021
feefladder pushed a commit to feefladder/pandas that referenced this pull request Jun 26, 2021
feefladder pushed a commit to feefladder/pandas that referenced this pull request Jun 26, 2021
@github-actions
Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Aug 12, 2021
@feefladder
Copy link
Contributor Author

This is still under discussion

@lithomas1 lithomas1 added Enhancement IO Excel read_excel, to_excel Needs Review Waiting for review/response from a maintainer. and removed Stale labels Aug 16, 2021
@rhshadrach
Copy link
Member

(Most of the) Original discussion: #40231 (comment)
Summary of why this is a regression: #42221 (comment)

As back in #40231, I'm +1 on having this as a feature, and even more so since it was identified that PR actually removed the feature in question.

@feefladder
Copy link
Contributor Author

feefladder commented Aug 18, 2021

The original opposition to the 'overwrite'/'write_to': #40231 (comment)
Basically that it is a mutating method that is not provided somewhere else.
But append mode is also only provided here (AFAIK, could be wrong). So you're already mutating the workbook, why would mutating sheets then be a problem.
On top of that, it was possible previously #42221

pandas/io/excel/_base.py Outdated Show resolved Hide resolved
@jreback jreback added this to the 1.4 milestone Aug 21, 2021
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

pls add a release note in 1.4.0 in the enhancements section

pandas/io/excel/_base.py Outdated Show resolved Hide resolved
pandas/io/excel/_base.py Outdated Show resolved Hide resolved
pandas/io/excel/_base.py Outdated Show resolved Hide resolved
pandas/io/excel/_base.py Outdated Show resolved Hide resolved
@jreback
Copy link
Contributor

jreback commented Aug 31, 2021

can you rebase

@pep8speaks
Copy link

pep8speaks commented Sep 7, 2021

Hello @joeperdefloep! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-11-15 15:26:58 UTC

@feefladder
Copy link
Contributor Author

I messed up a bit while rebasing and had to force-push. apologies for that :/

@feefladder
Copy link
Contributor Author

I really do not understand why the test fails. It says it fails doctests in _base.py, without mentioning a reason...

pandas/io/excel/_base.py Show resolved Hide resolved
pandas/io/excel/_base.py Outdated Show resolved Hide resolved
pandas/io/excel/_base.py Outdated Show resolved Hide resolved
pandas/io/excel/_base.py Show resolved Hide resolved
@lithomas1 lithomas1 removed the Needs Review Waiting for review/response from a maintainer. label Sep 10, 2021
@jreback jreback changed the title ENH: add if_sheet_exists='write_to' to ExcelWriter ENH: add if_sheet_exists='overlay' to ExcelWriter Sep 10, 2021
@rhshadrach
Copy link
Member

@joeperdefloep - still some docstring issues on the CI. ping if you're not sure how to fix

/home/runner/work/pandas/pandas/pandas/io/excel/_base.py:docstring of pandas.io.excel._base.ExcelWriter:52: WARNING: Explicit markup ends without a blank line; unexpected unindent.
looking for now-outdated files... none found
/home/runner/work/pandas/pandas/pandas/io/excel/_base.py:docstring of pandas.io.excel._base.ExcelWriter:53: WARNING: Bullet list ends without a blank line; unexpected unindent.

@feefladder
Copy link
Contributor Author

feefladder commented Sep 20, 2021

@rhshadrach shuold be good. Small question:
How did you get that output? I tried $ python scripts/validate_docstrings.py pandas.ExcelWriter
but got NameError due to ExcelWriter. Also in #43445.

these errors you see are definitely because of these lines. I wanted to have only the overlay option as a versionadded 1.4.0 But was not sure how to do that. I just made a versionchanged 1.4.0

@rhshadrach
Copy link
Member

If you click on Details unde the "Some checks were not successful", you can dig into the CI output.

@jreback
Copy link
Contributor

jreback commented Oct 3, 2021

@joeperdefloep can you merge master and address test failures

pandas/io/excel/_base.py Show resolved Hide resolved
pandas/io/excel/_base.py Outdated Show resolved Hide resolved
pandas/io/excel/_openpyxl.py Show resolved Hide resolved
pandas/tests/io/excel/test_openpyxl.py Show resolved Hide resolved
@feefladder
Copy link
Contributor Author

Sorry it took so long.... Done now :)

Copy link
Member

@lithomas1 lithomas1 left a comment

Choose a reason for hiding this comment

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

Can you add a whatsnew note(this is changing API so should be needed)?

doc/source/whatsnew/v1.4.0.rst Outdated Show resolved Hide resolved
pandas/tests/io/excel/test_openpyxl.py Outdated Show resolved Hide resolved
... if_sheet_exists="overlay",
... ) as writer:
... df1.to_excel(writer, sheet_name="Sheet1")
... df2.to_excel(writer, sheet_name="Sheet1", startcol=3)
Copy link
Contributor

Choose a reason for hiding this comment

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

does overlay require startrow to be not 1? (e.g should we error if this is not specified)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without startrow (e.g. startrow=0) it will overwrite the headers. This could be desired behaviour so I think we should not error.

Copy link
Member

Choose a reason for hiding this comment

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

This is what I would expect - unspecified starts at the very top left corner of the sheet.

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

lgtm

... if_sheet_exists="overlay",
... ) as writer:
... df1.to_excel(writer, sheet_name="Sheet1")
... df2.to_excel(writer, sheet_name="Sheet1", startcol=3)
Copy link
Member

Choose a reason for hiding this comment

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

This is what I would expect - unspecified starts at the very top left corner of the sheet.

@rhshadrach
Copy link
Member

@joeperdefloep - Can you resolve the conflicts.

@rhshadrach
Copy link
Member

@jreback - friendly ping.

@jreback jreback merged commit 80372bb into pandas-dev:master Nov 17, 2021
@jreback
Copy link
Contributor

jreback commented Nov 17, 2021

thanks @joeperdefloep very nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement IO Excel read_excel, to_excel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: #40231 does not allow writing multiple dataframes to a single sheet
6 participants