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

bpo-26175: Implement io.IOBase interface for SpooledTemporaryFile #29560

Merged

Conversation

pR0Ps
Copy link
Contributor

@pR0Ps pR0Ps commented Nov 15, 2021

Since the underlying file-like objects (either io.BytesIO, io.StringIO, or a true file object) all implement the io.IOBase interface, the SpooledTemporaryFile should as well.

Additionally, since the underlying file object will either be an instance of an io.BufferedIOBase (for binary mode) or an io.TextIOBase (for text mode), methods for these classes were also implemented.

In every case, the required methods and properties are simply delegated to the underlying file object.

Co-authored-by: Gary Fernie <[email protected]>

This is a rebased and reworked followup of the seemingly-abandoned #3249

https://bugs.python.org/issue26175

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@pR0Ps

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@ztane
Copy link
Contributor

ztane commented Nov 15, 2021

@pR0Ps I guess you ignored the change requests from the other PR, shouldn't call __del__ like that; see here.

Also IMO you'd want to give the warning regardless of the file having been rolled over to disk or not. BytesIO does not warn in its __del__. Otherwise you'd have warnings that are triggered by the phase of moon.

@pR0Ps pR0Ps force-pushed the bpo-26175_fix-SpooledTemporaryFile-IOBase branch 2 times, most recently from a39b1e8 to 54b6c7b Compare November 16, 2021 01:15
@pR0Ps
Copy link
Contributor Author

pR0Ps commented Nov 16, 2021

@ztane Made the requested change to the __del__ method and added a test to ensure that a ResourceWarning is generated in both a rolled and unrolled state.

@pR0Ps pR0Ps force-pushed the bpo-26175_fix-SpooledTemporaryFile-IOBase branch from 54b6c7b to fba27e4 Compare December 6, 2021 01:26
@pR0Ps
Copy link
Contributor Author

pR0Ps commented Dec 6, 2021

Rebased on latest main.

@pR0Ps pR0Ps force-pushed the bpo-26175_fix-SpooledTemporaryFile-IOBase branch from fba27e4 to c6546f7 Compare December 13, 2021 04:05
@pR0Ps pR0Ps force-pushed the bpo-26175_fix-SpooledTemporaryFile-IOBase branch from c6546f7 to ebb1d76 Compare December 21, 2021 18:05
@pR0Ps pR0Ps force-pushed the bpo-26175_fix-SpooledTemporaryFile-IOBase branch from ebb1d76 to dbc85fd Compare December 31, 2021 05:18
Since the underlying file-like objects (either `io.BytesIO`,
`io.StringIO`, or a true file object) all implement the `io.IOBase`
interface, the `SpooledTemporaryFile` should as well.

Additionally, since the underlying file object will either be an
instance of an `io.BufferedIOBase` (for binary mode) or an
`io.TextIOBase` (for text mode), methods for these classes were also
implemented.

In every case, the required methods and properties are simply delegated
to the underlying file object.

Co-authored-by: Gary Fernie <[email protected]>
@pR0Ps pR0Ps force-pushed the bpo-26175_fix-SpooledTemporaryFile-IOBase branch from dbc85fd to e66d4a3 Compare January 3, 2022 01:19
@merwok
Copy link
Member

merwok commented Jan 3, 2022

In the future, please avoid force pushes on Python repositories, they make reviewing less pleasant: https://devguide.python.org/pullrequest/

@pR0Ps pR0Ps force-pushed the bpo-26175_fix-SpooledTemporaryFile-IOBase branch from e66d4a3 to 753aeb0 Compare January 3, 2022 02:22
@pR0Ps
Copy link
Contributor Author

pR0Ps commented Jan 3, 2022

In the future, please avoid force pushes on Python repositories, they make reviewing less pleasant: https://devguide.python.org/pullrequest/

Sorry, I've re-pushed the change as a separate commit and will do the same in the future.

@arhadthedev
Copy link
Member

@pR0Ps Thanks! When PR is updated with a usual push, everybody subscribed can open the PR from "Unread notifications" and see only these updates of a diff. For example, a full PR4881073 contains 38 insertions but I do not need to reread it from a scratch after a (bunch of) commit(s) was added today:

image

Source: https://github.com/python/cpython/pull/29052/files/91f8ad6445a0671af6b0ee65cfba778c7ac150c4..d28a4ed7e24263972549ab82e6c7cf455e05defc

With a force-push, however, GitHub gives up and forces a reviewer to open a list of commits and manually scan for all changes:

image

When there are a few commits per day, it's ok. However, in this repo with a dosen of changes per hour, such manual works would become a second job.

@pR0Ps
Copy link
Contributor Author

pR0Ps commented May 2, 2022

@erlend-aasland Can you take a look please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants