Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed thePSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username onbugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please followthe steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Thanks for the feedback@merwok

I've made some changes to more accurately test howSpooledTemporaryFile implementsIOBase. So, there are now two separate tests:

• Assert that an instance ofSpooledTemporaryFile is an instance ofIOBase. BecauseIOBase already implements the interface (nearly) this makes it redundant to check for the individual attributes thatSpooledTemporaryFile should implement.
• Assert that the attributes which not declared inIOBase but should be implemented by subclasses are implemented bySpooledTemporaryFile.

From theIOBase docs:

Even though IOBase does not declare read(), readinto(), or write() because their signatures will vary, implementations and clients should consider those methods part of the interface.

I didn't expect the Spanish Inquisition!

Nobody expects the Spanish Inquisition!

@merwok: please review the changes made to this pull request.

There is at least one more incompatibility withIOBase:seek should return the current position instead ofNone.

You are right,@embe; it's part of the interface:
https://docs.python.org/library/io.html#io.IOBase.seek

The method now returns the value returned fromself._file.seek(), delegating the responsibility of returning the current new absolute file position to the underlying file.

Thanks for pointing this out

Re seek: I've also done the same withtruncate, as per Martin's recommendations on the Python bug tracker:https://bugs.python.org/issue26175#msg319145

This is also part of the file interface:
https://docs.python.org/library/io.html#io.IOBase.truncate

@pitrou I've considered your comments on what we should expect when calling__del__. There's three options which have been discussed:

• Do nothing. The underlying buffer or temp file will be deleted when the object is finalised anyway. If the object is deleted withdel, this would happen immediately. However, if__del__ is called then this would delay the deletion of the underlying buffer or temp file until the object is deleted withdel or is garbage collected, so possibly never. Here, we are in a situation where we called__del__, expected the resources to be freed, but not even a warning was raised.
• Inherit theio.IOBase.__del__ implementation. The underlying buffer or temp file will be deleted when__del__ is called, the object is deleted withdel, or the object is garbage collected. This is the expected behaviour, but base implementation will explicitly call close, suppressing theResourceWarning which should be raised when the object is deleted before closing the file.
• Delegate the__del__ call to the underlying file. If it is a buffer then then the buffer will be freed and if it's rolled over to disk then the temp file will be deleted. If the temp file was not closed before the object was deleted then aResourceWarning will be raised, just likeTemporaryFile. This ensures that resources are freed exactly when we expect them to be and also that we are raising a warning when resources are not being managed properly by the programmer.

I've picked the third option, as you suggested. Thanks for your review.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@merwok,@pitrou: please review the changes made to this pull request.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@GFernie, thank you for your last comments and for addressing the code review. It looks like@pitrou had some additional notes after that, so it would be great if you could look at this again. Thank you!

Not sure if@GFernie is still around -- are the outstanding changes/questions still those raised by@pitrou?@merwok?

This PR needs to merge master and address Antoine’s latest review.

@danieldjewell, you're right. Would you like to create a new PR based on this one? If so, please follow this section of thedevguide to give GFernie the credit as the original author. Thanks!

Hello,

For anyone that got here looking for the error'SpooledTemporaryFile' object has no attribute 'seekable'

In my case it was trying to pass anSpooledTemporaryFile to anZipFile to decompress it (Starlette's UploadFile class if anyone cares). But there are other cases. Until the issue referenced in this PR is fixed, an option is to monkey patch the class. Sorry, ugly hack follows.

In one of your modules:

fromtempfileimportSpooledTemporaryFiledef_seekable(self):"""    Monkey patched seekable() method for the SpooledTemporaryFile class.    Sadly, we cannot send an instance of the SpooledTemporaryFile to the    ZipFile object.    This is because the SpooledTemporaryFile doesn't inherit / implement the    IOBase class.    This bug is reported in CPython:        https://bugs.python.org/issue26175    And an unmerged PR is located here:        https://github.com/python/cpython/pull/3249/files    In Python 3.8, attempting to pass the tmpfd fails with:        AttributeError:            'SpooledTemporaryFile' object has no attribute 'seekable'    So, the workaround to avoid copying the file is to monkey patch the class    and add the missing method. Sorry.    """returnself._file.seekable()SpooledTemporaryFile.seekable=_seekable

To Google people: this also breaks pandas.read_csv() and this monkey patch can be used for property readable writable and seekable to fix it.

Hello,

For anyone that got here looking for the error'SpooledTemporaryFile' object has no attribute 'seekable'

In my case it was trying to pass anSpooledTemporaryFile to anZipFile to decompress it (Starlette's UploadFile class if anyone cares). But there are other cases. Until the issue referenced in this PR is fixed, an option is to monkey patch the class. Sorry, ugly hack follows.

In one of your modules:

fromtempfileimportSpooledTemporaryFiledef_seekable(self):"""    Monkey patched seekable() method for the SpooledTemporaryFile class.    Sadly, we cannot send an instance of the SpooledTemporaryFile to the    ZipFile object.    This is because the SpooledTemporaryFile doesn't inherit / implement the    IOBase class.    This bug is reported in CPython:        https://bugs.python.org/issue26175    And an unmerged PR is located here:        https://github.com/python/cpython/pull/3249/files    In Python 3.8, attempting to pass the tmpfd fails with:        AttributeError:            'SpooledTemporaryFile' object has no attribute 'seekable'    So, the workaround to avoid copying the file is to monkey patch the class    and add the missing method. Sorry.    """returnself._file.seekable()SpooledTemporaryFile.seekable=_seekable

Python dev here from the future (2021 - save yourselves while you can). Still gettingSpooledTemporaryFile errors while reading CSVs (via tempfiles) in. Kindly asking if requested changes will be addressed@GFernie ?

@MikeEcho since it seemed like@GFernie had moved on I submitted an updated and rebased PR for this fix in#29560 with all of the code review comments addressed. Just waiting on a code review.

This PR is stale because it has been open for 30 days with no activity.

Close this because I merged#29560.