-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
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 34 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 |
---|---|---|
|
@@ -87,6 +87,7 @@ Other enhancements | |
- :meth:`DataFrame.applymap` now uses the :meth:`~api.extensions.ExtensionArray.map` method of underlying :class:`api.extensions.ExtensionArray` instances (:issue:`52219`) | ||
- :meth:`arrays.SparseArray.map` now supports ``na_action`` (:issue:`52096`). | ||
rmhowe425 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Add dtype of categories to ``repr`` information of :class:`CategoricalDtype` (:issue:`52179`) | ||
- Adding ``engine_kwargs`` parameter to :meth:`DataFrame.read_excel` (:issue:`52214`) | ||
- | ||
|
||
.. --------------------------------------------------------------------------- | ||
|
@@ -339,7 +340,6 @@ Period | |
- Bug in :meth:`arrays.PeriodArray.map` and :meth:`PeriodIndex.map`, where the supplied callable operated array-wise instead of element-wise (:issue:`51977`) | ||
- Bug in :func:`read_csv` not processing empty strings as a null value, with ``engine="pyarrow"`` (:issue:`52087`) | ||
- Bug in :func:`read_csv` returning ``object`` dtype columns instead of ``float64`` dtype columns with ``engine="pyarrow"`` for columns that are all null with ``engine="pyarrow"`` (:issue:`52087`) | ||
- Bug in incorrectly allowing construction of :class:`Period` or :class:`PeriodDtype` with :class:`CustomBusinessDay` freq; use :class:`BusinessDay` instead (:issue:`52534`) | ||
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. something went wrong when merging here, I presume you didn't mean to remove this line? 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. @MarcoGorelli Last night after I pushed the new whatsnew file there was a PR conflict and I removed that entry to fix the conflict. Shouldn't my PR only contain my contribution? Should I have added that entry to fix the conflict? 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.
yes, exactly - whereas currently, it's showing that you also deleted another line please check https://github.com/pandas-dev/pandas/pull/52214/files to verify that the PR only contains your changes 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. @MarcoGorelli Looking at that URL, it looks correct to me. The PR only contains my changes. However, when I run into merge conflicts after other people's PRs are approved, do I need to add their changes when I examine the merge conflict diff, or remove them? In this case I removed them, which is why the line below is shaded in red. Previously I added them and that seemed to cause problems as well. hmmmm Bug in incorrectly allowing construction of :class: 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. depends on the merge conflict - if it's a whatsnew note, you probably need to select "keep both changes" 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. Understood! Thank you for the clarity! I'll add the above entry back and keep this in mind moving forward! |
||
- | ||
|
||
Plotting | ||
|
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`" | ||
|
Uh oh!
There was an error while loading. Please reload this page.