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

MAINT: rename IOError -> OSError #43366

Merged
merged 4 commits into from
Sep 11, 2021
Merged

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Sep 2, 2021

Since Python 3.3, IO and OS exceptions were re-worked as per PEP 3151. The new main base class is OSError, with several other exceptions merged, including IOError (i.e. assert IOError is OSError).

This PR changes many of the old alias to OSError, with a few exceptions:

  • Versioneer EnvironmentError were not modified (this change has been merged upstream)
  • In pandas/_libs/parsers.pyx, this is not an OSError, as there is no issue reading the file. The exception is reclassified as a TypeError.

Are any of these changes worthy of a whatsnew entry?

Copy link
Member

@twoertwein twoertwein left a comment

Choose a reason for hiding this comment

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

Thank you! The changes look good to me.

@jreback jreback added the Error Reporting Incorrect or improved errors from pandas label Sep 4, 2021
@jreback jreback added this to the 1.4 milestone Sep 4, 2021
@jreback
Copy link
Contributor

jreback commented Sep 4, 2021

@mwtoews can you add a whatsnew note in 1.4., I/O Bug fix section is fine, just noting that we changed this. (and ref this issue). pls merge master and ping on green.

@mwtoews
Copy link
Contributor Author

mwtoews commented Sep 5, 2021

@jreback added a whatsnew entry to describe the user-visible change, let me know if it suites or needs rephrasing. I don't understand the Azure "The operation was canceled" CI failure.

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 resolve conflicts? Don't worry about the timeouts , they're flaky.

doc/source/whatsnew/v1.4.0.rst Outdated Show resolved Hide resolved
pandas/_libs/parsers.pyx Outdated Show resolved Hide resolved
@mwtoews mwtoews changed the title MAINT: rename IOError -> OSError (or TypeError in TextReader) MAINT: rename IOError -> OSError Sep 7, 2021
@mwtoews
Copy link
Contributor Author

mwtoews commented Sep 7, 2021

The scope of this PR has been reduce to a pure maintenance change, which shouldn't need any whatsnew entry, since any IOError class thrown was viewed as OSError to the user, so no changes should be visible.

The more involved bug fix that changes the exception class, and works across different parsers is at #43443

pandas/io/common.py Outdated Show resolved Hide resolved
@alimcmaster1
Copy link
Member

@mwtoews - can you take a look at the test failures?

@mwtoews
Copy link
Contributor Author

mwtoews commented Sep 9, 2021

The third commit in this PR fixes the excel readers, which allow bytes for the first parameter (e.g. pandas.read_excel). Since get_handle expects FilePathOrBuffer, these bytes need to be passed as BytesIO buffers.

I'd appreciate a review from anyone familiar with pandas/io/excel/_base.py. As these are internal changes to the excel readers, I'm not expecting any further whatsnew notes on this aspect, but please correct me if it should.

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.

@mwtoews
Copy link
Contributor Author

mwtoews commented Sep 9, 2021

It seems that support for reading bytes for the excel readers was added with #30519

Also, feel free to re-title and re-classify this PR, as the scope has shifted since initially submitted.

@jreback
Copy link
Contributor

jreback commented Sep 9, 2021

It seems that support for reading bytes for the excel readers was added with #30519

Also, feel free to re-title and re-classify this PR, as the scope has shifted since initially submitted.

no its fine, just the question on where the bytes are wrapped is still likely hit

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.

excel changes lgtm - verified tests would fail without the changes in excel/_base

@jreback
Copy link
Contributor

jreback commented Sep 11, 2021

@lithomas1
Copy link
Member

@github-actions pre-commit

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.

LGTM. and thanks for the PR (I took care of the pre-commit issue for you).

P.S. You don't have to force-push/squash your commits if you don't want to. We squash on merge so it's fine.

@lithomas1 lithomas1 closed this Sep 11, 2021
@lithomas1 lithomas1 reopened this Sep 11, 2021
@lithomas1 lithomas1 merged commit 5f36af3 into pandas-dev:master Sep 11, 2021
@lithomas1
Copy link
Member

thanks @mwtoews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants