-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
ENH: Adding engine_kwargs to Excel engines for issue #40274 #52214
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
Changes from 29 commits
817199f
1333165
1cc54cd
8391425
bec1da2
c0988d6
2267d30
db13a39
229954e
14b4be0
057d5a2
c05f182
9065261
45589bb
93c6e60
d60aa97
5431178
f631de7
1000a30
86dbb35
d2eae07
da022c8
8106cc6
19a6d88
c69ef91
242765d
c9aa28a
46be9ec
cef90f4
f692c8e
96c6fe0
0391c9f
f2c8e2a
af55880
679ab4b
f9be828
3412af0
f379120
8d7933c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
import os | ||
from pathlib import Path | ||
import platform | ||
import re | ||
from urllib.error import URLError | ||
from zipfile import BadZipFile | ||
|
||
|
@@ -148,6 +149,33 @@ def parser(self, *args, **kwargs): | |
expected = expected_defaults[read_ext[1:]] | ||
assert result == expected | ||
|
||
def test_engine_kwargs(self, read_ext, engine): | ||
# GH#52214 | ||
expected_defaults = { | ||
rhshadrach marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"xlsx": {"foo": "abcd"}, | ||
"xlsm": {"foo": 123}, | ||
"xlsb": {"foo": "True"}, | ||
"xls": {"foo": True}, | ||
"ods": {"foo": "abcd"}, | ||
} | ||
|
||
msg = re.escape(r"load_workbook() got an unexpected keyword argument 'foo'") | ||
|
||
if read_ext[1:] == "xls" or read_ext[1:] == "xlsb": | ||
msg = re.escape(r"open_workbook() got an unexpected keyword argument 'foo'") | ||
|
||
elif read_ext[1:] == "ods": | ||
msg = re.escape(r"load() got an unexpected keyword argument 'foo'") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the extra newlines makes this hard to read, how about if read_ext[1:] == "xls" or read_ext[1:] == "xlsb":
msg = ...
elif read_ext[1:] == "ods":
msg = ...
else:
msg = ... ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed! |
||
|
||
if engine is not None and expected_defaults[read_ext[1:]]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessary! Fixed! |
||
with pytest.raises(TypeError, match=msg): | ||
pd.read_excel( | ||
"test1" + read_ext, | ||
sheet_name="Sheet1", | ||
index_col=0, | ||
engine_kwargs=expected_defaults[read_ext[1:]], | ||
) | ||
|
||
def test_usecols_int(self, read_ext): | ||
# usecols as int | ||
msg = "Passing an integer for `usecols`" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are a lot of unrelated entries modified in this diff? I think only one entry should have been added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mroeschke Mistakes caused by handling merge conflicts. They should have been fixed from previous comments left by reviewers. Are we still seeing issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally for this pull request we should just see one new entry but other entries in this diff appear changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mroeschke I'll pull the latest version of
whatsnew/v2.1.0.rst
from master, add my change for this PR and push. That should guarantee that all incorrect modifications to the file have been fixedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mroeschke Updated
whatsnew/v2.1.0.rst
should be good