Skip to content

CLN: Share code between _BytesTarFile and _BytesZipFile #47153

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 5 commits into from
May 30, 2022
Merged

CLN: Share code between _BytesTarFile and _BytesZipFile #47153

merged 5 commits into from
May 30, 2022

Conversation

twoertwein
Copy link
Member

@twoertwein twoertwein commented May 27, 2022 •
edited
Loading

Needs some more work (test case + whatsnew) but want to first check whether the CI is happy. _BytesTarFile caused ValueErrors (due to inheriting from two IO classes?).

Would also like to further clean up _BytesTarFile and _BytesZipFile but that might be a separate PR.

@twoertwein twoertwein marked this pull request as ready for review May 28, 2022 00:50
@twoertwein twoertwein added IO Data IO issues that don't fit into a more specific label Error Reporting Incorrect or improved errors from pandas labels May 28, 2022
simonjayhawkins
simonjayhawkins reviewed May 28, 2022
View reviewed changes
doc/source/whatsnew/v1.5.0.rst Outdated
@@ -814,6 +814,7 @@ I/O
- Bug in :func:`read_excel` when reading a ``.ods`` file with newlines between xml elements (:issue:`45598`)
- Bug in :func:`read_parquet` when ``engine="fastparquet"`` where the file was not closed on error (:issue:`46555`)
- :meth:`to_html` now excludes the ``border`` attribute from ``<table>`` elements when ``border`` keyword is set to ``False``.
- Most I/O methods do no longer suppress ``OSError`` and ``ValueError`` when closing file handles (:issue:`47136`)
Copy link
Member

Choose a reason for hiding this comment

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

see #47136 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

If it should go for 1.4, I can make a PR just for 1.4. Literally just removing the try-catch (1.4 doesn't have the problematic tar support; and the cleanup doesn't need to be part of 1.4 either).

Copy link
Contributor

Choose a reason for hiding this comment

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

prob worth a targeted patch for 1.4

Copy link
Member Author

Choose a reason for hiding this comment

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

opened #47160. Will move the whatsnew to 1.4.3 in this PR to keep the PRs in sync.

@twoertwein twoertwein mentioned this pull request May 28, 2022
Closed
simonjayhawkins
simonjayhawkins requested changes May 29, 2022
View reviewed changes
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

#47160 should be merged first

@simonjayhawkins simonjayhawkins added this to the 1.5 milestone May 29, 2022
@twoertwein
Copy link
Member Author

Will submit a bug-fix after this PR is merged.

jreback
jreback approved these changes May 29, 2022
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.

ok let's merge this, then do the appropriate backport (also prob need a whats new for 1.5 / different one for 1.4.3)

@jreback
Copy link
Contributor

jreback commented May 29, 2022

can you add some minimal testing for this & whatsnew note

@twoertwein
Copy link
Member Author

ok let's merge this, then do the appropriate backport (also prob need a whats new for 1.5 / different one for 1.4.3)

It fixed a bug in the not-yet-released tar feature. Does this need a whatsnew "even better tar support ;)"?

Will add a test.

@jreback
Copy link
Contributor

jreback commented May 29, 2022

right the note is about the propagating closing errors (don't need anything for changes on unrejeased)

@twoertwein
Copy link
Member Author

right the note is about the propagating closing errors (don't need anything for changes on unrejeased)

I changed this PR to still catch the error (if used within get_handle, the test calls the class directly). A follow-up PR (for main+1.4) will then remove the try-catch. See #47160 (comment)

@twoertwein twoertwein mentioned this pull request May 29, 2022
Merged
@twoertwein twoertwein removed the Error Reporting Incorrect or improved errors from pandas label May 29, 2022
@simonjayhawkins
Copy link
Member

I changed this PR to still catch the error

should probably update title.

@simonjayhawkins simonjayhawkins added Refactor Internal refactoring of code Clean labels May 30, 2022
@simonjayhawkins
Copy link
Member

I changed this PR to still catch the error

should probably update title.

@twoertwein can you do this and then merge when ready.

@twoertwein twoertwein changed the title CLN: do not suppress errors when closing file handles CLN: Share code between _BytesTarFile and _BytesZipFile May 30, 2022
@twoertwein twoertwein merged commit 8e94afb into pandas-dev:main May 30, 2022
@twoertwein twoertwein deleted the io branch June 8, 2022 19:26
yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
@twoertwein @yehoshuadimarsky
CLN: Share code between _BytesTarFile and _BytesZipFile (pandas-dev#4…
530f844 
…7153)

* CLN: do not suppress errors when closing file handles

* more cleanups

* move to 1.4.3

* only cleanup

* add test case
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

@simonjayhawkins simonjayhawkins simonjayhawkins approved these changes

Assignees
No one assigned
Labels
Clean IO Data IO issues that don't fit into a more specific label Refactor Internal refactoring of code
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.
3 participants
@twoertwein @jreback @simonjayhawkins