Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 35.2k
bpo-26175: Fix SpooledTemporaryFile IOBase abstract#3249
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
83d61523b69baa753b4fba70113efb28362bc9d0a9b588decab49dd14251b2119f4c084519b9c554aad6be094b3File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -620,7 +620,7 @@ def TemporaryFile(mode='w+b', buffering=-1, encoding=None, | ||
| _os.close(fd) | ||
| raise | ||
| class SpooledTemporaryFile: | ||
| class SpooledTemporaryFile(_io.IOBase): | ||
| """Temporary file wrapper, specialized to switch from BytesIO | ||
| or StringIO to a real file when it exceeds a certain size or | ||
| when a fileno is needed. | ||
| @@ -679,6 +679,9 @@ def __exit__(self, exc, value, tb): | ||
| def __iter__(self): | ||
| return self._file.__iter__() | ||
| def __del__(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't be added: deleting the SpooledTemporaryFile will null out the reference to the underline file, and therefor call its
| ||
| return self._file.__del__() | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, calling the def__del__(self):
ifnotself.closed:
importwarningswarnings.warn('unclosed file %r'% (self,), ResourceWarning,
stacklevel=2, source=self)
self.close()(this is adapted from | ||
| def close(self): | ||
| self._file.close() | ||
| @@ -725,14 +728,23 @@ def newlines(self): | ||
| def read(self, *args): | ||
| return self._file.read(*args) | ||
| def readable(self): | ||
| return self._file.readable() | ||
| def readinto(self, b): | ||
| return self._file.readinto(b) | ||
| def readline(self, *args): | ||
| return self._file.readline(*args) | ||
| def readlines(self, *args): | ||
| return self._file.readlines(*args) | ||
| def seek(self, *args): | ||
| self._file.seek(*args) | ||
| return self._file.seek(*args) | ||
| def seekable(self): | ||
| return self._file.seekable() | ||
| @property | ||
| def softspace(self): | ||
| @@ -743,18 +755,21 @@ def tell(self): | ||
| def truncate(self, size=None): | ||
| if size is None: | ||
| self._file.truncate() | ||
| return self._file.truncate() | ||
| else: | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this else is not needed, PEP warning, might as well fix Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not necessary to remove the else. It does no harm. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I never said it was necessary, just bad style as it unnecessarily increased the code depth, take my comment as an FYI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just wanted to confirm @merwok - nothing needs to be changed here for approval? Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I advise to not change the | ||
| if size > self._max_size: | ||
| self.rollover() | ||
| self._file.truncate(size) | ||
| return self._file.truncate(size) | ||
| def write(self, s): | ||
| file = self._file | ||
| rv = file.write(s) | ||
| self._check(file) | ||
| return rv | ||
| def writable(self): | ||
| return self._file.writable() | ||
| def writelines(self, iterable): | ||
| file = self._file | ||
| rv = file.writelines(iterable) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -985,6 +985,22 @@ def test_basic(self): | ||
| f = self.do_create(max_size=100, pre="a", suf=".txt") | ||
| self.assertFalse(f._rolled) | ||
| def test_is_iobase(self): | ||
| # SpooledTemporaryFile should implement io.IOBase | ||
| self.assertIsInstance(self.do_create(), io.IOBase) | ||
| def test_iobase_interface(self): | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those tests are not very interesting. It would be better to test the methods operate properly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It literally delegates everything to the underlying file. The existing tests should cover the behaviors. The new behaviors are intended to flesh out the API so that | ||
| # SpooledTemporaryFile should implement the io.IOBase interface. | ||
| # IOBase does not declare read(), readinto(), or write(), but | ||
| # they should be considered part of the interface. | ||
| iobase_abstract = {'read', 'readinto', 'write'} | ||
| spooledtempfile_abstract = set(dir(tempfile.SpooledTemporaryFile)) | ||
| missing_attributes = iobase_abstract - spooledtempfile_abstract | ||
| self.assertFalse( | ||
| missing_attributes, | ||
| 'io.IOBase attributes missing from SpooledTemporaryFile' | ||
| ) | ||
| def test_del_on_close(self): | ||
| # A SpooledTemporaryFile is deleted when closed | ||
| dir = tempfile.mkdtemp() | ||
| @@ -1000,6 +1016,21 @@ def test_del_on_close(self): | ||
| finally: | ||
| os.rmdir(dir) | ||
| def test_del_rolled_file(self): | ||
| # The rolled file should be deleted when the | ||
| # SpooledTemporaryFile object is deleted. This should raise a | ||
| # ResourceWarning since the file was not explicitly closed. | ||
| f = self.do_create(max_size=2) | ||
| f.write(b'foo') | ||
| filename = f.name | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this actually work? Here I get something like: >>>f=tempfile.SpooledTemporaryFile()
>>>f.name>>>f.rollover()
>>>f.name3 | ||
| self.assertTrue(os.path.exists(filename)) | ||
| with self.assertWarns(ResourceWarning): | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we also expect a | ||
| f.__del__() | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not correct to call | ||
| self.assertFalse( | ||
| os.path.exists(filename), | ||
| "Rolled SpooledTemporaryFile not deleted after __del__" | ||
| ) | ||
| def test_rewrite_small(self): | ||
| # A SpooledTemporaryFile can be written to multiple within the max_size | ||
| f = self.do_create(max_size=30) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fully implement io.IOBase interface for tempfile.SpooledTemporaryFile. | ||
| Patch by Gary Fernie. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since 3.8.0 has been released, this should be moved to a new
versionchanged:: 3.9block.