Skip to content

REG: read_excel with engine specified raises on non-path/non-buffer #39586

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Feb 7, 2021
Merged

REG: read_excel with engine specified raises on non-path/non-buffer #39586

merged 11 commits into from
Feb 7, 2021

Conversation

rhshadrach
Copy link
Member

@rhshadrach rhshadrach added Regression Functionality that used to work in a prior pandas version IO Excel read_excel, to_excel labels Feb 3, 2021
@rhshadrach rhshadrach added this to the 1.2.2 milestone Feb 3, 2021
@twoertwein twoertwein mentioned this pull request Feb 4, 2021
Closed
4 tasks
jreback
jreback approved these changes Feb 4, 2021
View reviewed changes
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.

@twoertwein ?

twoertwein
twoertwein reviewed Feb 4, 2021
View reviewed changes
@twoertwein
Copy link
Member

looks good to me!

twoertwein
twoertwein reviewed Feb 4, 2021
View reviewed changes
pandas/tests/io/excel/test_openpyxl.py
filename = datapath("io", "data", "excel", "test1" + ext)
wb = openpyxl.load_workbook(filename)
result = pd.read_excel(wb, engine="openpyxl")
expected = pd.read_excel(filename)
Copy link
Member

Choose a reason for hiding this comment

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

It cannot hurt to call wb.close() before tm.assert_frame_equal

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @twoertwein, added.

@jreback
Copy link
Contributor

jreback commented Feb 5, 2021

if you can merge master and ping on greem

@rhshadrach
Merge branch 'master' of https://github.com/pandas-dev/pandas into op…
1f16602 
…enpyxl_workbook

� Conflicts:
�	doc/source/whatsnew/v1.2.2.rst
�	pandas/tests/io/excel/test_openpyxl.py
@rhshadrach
Added logic/tests for determining if a sheet is read-only
ba2bc75
@rhshadrach
Added comment
1381ecc
@rhshadrach
Copy link
Member Author

This PR enables the possibility that the sheet in get_sheet_data is not read-only. We only want to apply the fix from #39486 in the case the sheet is read-only (which is always the case when reading from a path or buffer). Unfortunately it seems to me that openpyxl provides no way of telling if a sheet is read-only or not; for this I've opened an issue upstream.

The method reset_dimensions only exists on read-only sheets, and so it can be used for this purpose (even if not ideal).

I've added tests for this and to ensure the fix from #39486 works on non-read-only workbooks.

@rhshadrach
Copy link
Member Author

@jreback - green

@jreback jreback merged commit 95ee0b6 into pandas-dev:master Feb 7, 2021
@jreback
Copy link
Contributor

jreback commented Feb 7, 2021

thanks @rhshadrach

@jreback
Copy link
Contributor

jreback commented Feb 7, 2021

@meeseeksdev backport 1.2.x

@lumberbot-app

This comment has been minimized.

Sign in to view
@jreback
Copy link
Contributor

jreback commented Feb 7, 2021

cc @simonjayhawkins

@simonjayhawkins
Copy link
Member

@rhshadrach There have been some changes to master since 1.2 that were not backported (#38710 and #39008) and trying to apply these changes, I'm getting

=================================================== short test summary info ===================================================
FAILED pandas/tests/io/excel/test_readers.py::TestReaders::test_corrupt_bytes_raises[engine_and_read_ext0] - AttributeError:...
FAILED pandas/tests/io/excel/test_readers.py::TestExcelFileRead::test_excel_read_binary[engine_and_read_ext0] - AttributeErr...
================================== 2 failed, 1398 passed, 146 skipped, 10 xfailed in 25.65s ===================================

@simonjayhawkins
Copy link
Member

ahh , needed a content=

simonjayhawkins pushed a commit to simonjayhawkins/pandas that referenced this pull request Feb 7, 2021
@rhshadrach @simonjayhawkins
Backport PR pandas-dev#39586: REG: read_excel with engine specified r…
555c4d8 
…aises on non-path/non-buffer
@simonjayhawkins simonjayhawkins mentioned this pull request Feb 7, 2021
Merged
CyberQin pushed a commit to CyberQin/pandas that referenced this pull request Feb 8, 2021
@rhshadrach @CyberQin
REG: read_excel with engine specified raises on non-path/non-buffer (p…
a8cf281 
…andas-dev#39586)
simonjayhawkins added a commit that referenced this pull request Feb 8, 2021
@simonjayhawkins @rhshadrach
Backport PR #39586: REG: read_excel with engine specified raises on n…
3ebbc77 
…on-path/non-buffer (#39652)

Co-authored-by: Richard Shadrach <[email protected]>
@rhshadrach rhshadrach deleted the openpyxl_workbook branch February 12, 2021 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

+1 more reviewer

@twoertwein twoertwein twoertwein left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
IO Excel read_excel, to_excel Regression Functionality that used to work in a prior pandas version
Projects
None yet
Milestone
1.2.2
Development

Successfully merging this pull request may close these issues.

BUG: read_excel with Workbook and engine="openpyxl" raises ValueError
5 participants
@rhshadrach @twoertwein @jreback @simonjayhawkins @jorisvandenbossche