Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

bpo-26175: Implementio.IOBase interface forSpooledTemporaryFile#29560

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@pR0Ps
Copy link
Contributor

@pR0PspR0Ps commentedNov 15, 2021
edited
Loading

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

Additionally, since the underlying file object will either be an instance of anio.BufferedIOBase (for binary mode) or anio.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 <Gary.Fernie@skyscanner.net>

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

https://bugs.python.org/issue26175

adriangb and NicolaiSoeborg reacted with hooray emojiadriangb reacted with rocket emoji
@the-knights-who-say-ni

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 thePSF 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 followthe 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 cancheck 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 commentedNov 15, 2021
edited
Loading

@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 warningregardless 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.

@pR0PspR0Psforce-pushed thebpo-26175_fix-SpooledTemporaryFile-IOBase branch 2 times, most recently froma39b1e8 to54b6c7bCompareNovember 16, 2021 01:15
@pR0Ps
Copy link
ContributorAuthor

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

@pR0PspR0Psforce-pushed thebpo-26175_fix-SpooledTemporaryFile-IOBase branch from54b6c7b tofba27e4CompareDecember 6, 2021 01:26
@pR0Ps
Copy link
ContributorAuthor

Rebased on latest main.

@pR0PspR0Psforce-pushed thebpo-26175_fix-SpooledTemporaryFile-IOBase branch fromfba27e4 toc6546f7CompareDecember 13, 2021 04:05
@pR0PspR0Psforce-pushed thebpo-26175_fix-SpooledTemporaryFile-IOBase branch fromc6546f7 toebb1d76CompareDecember 21, 2021 18:05
@pR0PspR0Psforce-pushed thebpo-26175_fix-SpooledTemporaryFile-IOBase branch fromebb1d76 todbc85fdCompareDecember 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 aninstance of an `io.BufferedIOBase` (for binary mode) or an`io.TextIOBase` (for text mode), methods for these classes were alsoimplemented.In every case, the required methods and properties are simply delegatedto the underlying file object.Co-authored-by: Gary Fernie <Gary.Fernie@skyscanner.net>
@pR0PspR0Psforce-pushed thebpo-26175_fix-SpooledTemporaryFile-IOBase branch fromdbc85fd toe66d4a3CompareJanuary 3, 2022 01:19
@merwok
Copy link
Member

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

@pR0PspR0Psforce-pushed thebpo-26175_fix-SpooledTemporaryFile-IOBase branch frome66d4a3 to753aeb0CompareJanuary 3, 2022 02:22
@pR0Ps
Copy link
ContributorAuthor

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.

merwok reacted with thumbs up emoji

@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 fullPR4881073 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 reacted with thumbs up emoji

@pR0Ps
Copy link
ContributorAuthor

@erlend-aasland Can you take a look please?

@methanemethane merged commit78e70be intopython:mainMay 3, 2022
@pR0PspR0Ps deleted the bpo-26175_fix-SpooledTemporaryFile-IOBase branchMay 3, 2022 17:09
JelleZijlstra added a commit to python/typeshed that referenced this pull requestMay 18, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@merwokmerwokmerwok left review comments

@methanemethanemethane approved these changes

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@pR0Ps@the-knights-who-say-ni@ztane@merwok@arhadthedev@methane@bedevere-bot@AlexWaygood

[8]ページ先頭

©2009-2025 Movatter.jp