DEPR: Remove bytes input for read_excel #53830
Conversation
rhshadrach
added
IO Excel
pandas/io/excel/_base.py
Outdated
| @@ -97,6 +99,9 @@ | |||
| By file-like object, we refer to objects with a ``read()`` method, | |||
| such as a file handle (e.g. via builtin ``open`` function) | |||
| or ``StringIO``. | |||
|
|
|||
| .. deprecated:: 2.1.0 | |||
| Passing byte strings is deprecated. | |||
There was a problem hiding this comment.
Can you add the "wrap in a BytesIO object" here too
pandas/io/excel/_base.py
Outdated
| Passing byte strings is deprecated. To read from a " | ||
| "byte string, wrap it in a `BytesIO` object. |
There was a problem hiding this comment.
Remove the quotes ("), yea?
There was a problem hiding this comment.
oh shoot thanks! Fixed now!
pandas/io/excel/_base.py
Outdated
| Passing byte strings is deprecated. To read from a " | ||
| "byte string, wrap it in a `BytesIO` object. | ||
| Passing byte strings is deprecated. To read from a | ||
| byte string, wrap it in a `BytesIO` object. |
There was a problem hiding this comment.
Sorry - I missed this before. The API docs are autogenerated using sphinx. For sphinx, you need a double backtick here: ``BytesIO``.
|
@rhshadrach Pinging on green! |
| @@ -1503,7 +1509,13 @@ def __init__( | |||
|
|
|||
| # First argument can also be bytes, so create a buffer | |||
| if isinstance(path_or_buffer, bytes): | |||
| path_or_buffer = BytesIO(path_or_buffer) | |||
There was a problem hiding this comment.
To maintain the current behavior during the deprecation, I think we need to continue wrapping this in BytesIO
There was a problem hiding this comment.
Ah yep! I added this back. Thanks for catching this!
There was a problem hiding this comment.
It seems that test_excel_read_binary still passes without this line on main. If we are wanting to remove this functionality so that we raise when bytes are passed, I think more needs to be done. But I think this can wait until the deprecation is enforced.
There was a problem hiding this comment.
@rhshadrach I'm a bit confused on your above message 🤔
The test is passing without the line on main because the input to read_excel is already a BytesIO object. If I left the input as raw binary the test would fail.
There was a problem hiding this comment.
The test in question is
expected = pd.read_excel("test1" + read_ext, engine=engine)
with open("test1" + read_ext, "rb") as f:
data = f.read()
actual = pd.read_excel(data, engine=engine)
tm.assert_frame_equal(expected, actual)
data here is a bytes, not a BytesIO, object. This is passed directly to read_excel.
There was a problem hiding this comment.
@mroeschke has been helping me with a similar problem on a few other PRs.
Are you okay with it if I go ahead and take care of this discrepancy before we submit this one?
There was a problem hiding this comment.
I'm pretty sure the fix for this problem is just adding more input validation by using a handful of methods from common.py
There was a problem hiding this comment.
Agreed, I think we're okay to deprecate with this PR as is, this can wait until the deprecation is enforced (i.e. the feature of reading bytes is removed in pandas 3.0).
|
@rhshadrach Pinging on green again after I pushed the last time! |
|
Thanks again @rmhowe425 |
* Adding deprecation logic and updating documentation. * Adding PR number to new unit test * Updating read_excel() documentation per reviewer recommendations. * Updating read_excel() documentation per reviewer recommendations. * Updating read_excel() documentation per reviewer recommendations. * Updating implementation per reviewer recommendations.
read_excel,read_html, andread_xml#53767doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.