-
-
Notifications
You must be signed in to change notification settings - Fork 34.8k
gh-152586: Make tempfile.TemporaryFileWrapper public
#152646
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ | |
| "TMP_MAX", "gettempprefix", # constants | ||
| "tempdir", "gettempdir", | ||
| "gettempprefixb", "gettempdirb", | ||
| "TemporaryFileWrapper", | ||
| ] | ||
|
|
||
|
|
||
|
|
@@ -484,7 +485,7 @@ def __del__(self): | |
| _warnings.warn(self.warn_message, ResourceWarning) | ||
|
|
||
|
|
||
| class _TemporaryFileWrapper: | ||
| class TemporaryFileWrapper: | ||
| """Temporary file wrapper | ||
|
|
||
| This class provides a wrapper around files opened for | ||
|
|
@@ -555,6 +556,8 @@ def __iter__(self): | |
| for line in self.file: | ||
| yield line | ||
|
|
||
| _TemporaryFileWrapper = TemporaryFileWrapper | ||
|
|
||
| def NamedTemporaryFile(mode='w+b', buffering=-1, encoding=None, | ||
| newline=None, suffix=None, prefix=None, | ||
| dir=None, delete=True, *, errors=None, | ||
|
|
@@ -607,7 +610,7 @@ def opener(*args): | |
| raw = getattr(file, 'buffer', file) | ||
| raw = getattr(raw, 'raw', raw) | ||
| raw.name = name | ||
| return _TemporaryFileWrapper(file, name, delete, delete_on_close) | ||
| return TemporaryFileWrapper(file, name, delete, delete_on_close) | ||
|
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.
Contributor
Author
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. thanks for pointing out |
||
| except: | ||
| file.close() | ||
| raise | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,6 +148,7 @@ def test_exports(self): | |
| "template" : 1, | ||
| "SpooledTemporaryFile" : 1, | ||
| "TemporaryDirectory" : 1, | ||
| "TemporaryFileWrapper" : 1, | ||
| } | ||
|
|
||
| unexp = [] | ||
|
|
@@ -1141,7 +1142,7 @@ def my_func(dir): | |
| try: | ||
| with self.assertWarnsRegex( | ||
| expected_warning=ResourceWarning, | ||
| expected_regex=r"Implicitly cleaning up <_TemporaryFileWrapper file=.*>", | ||
| expected_regex=r"Implicitly cleaning up <TemporaryFileWrapper file=.*>", | ||
| ): | ||
| tmp_name = my_func(dir) | ||
| support.gc_collect() | ||
|
|
@@ -1195,6 +1196,19 @@ def test_unexpected_error(self): | |
|
|
||
| # How to test the mode and bufsize parameters? | ||
|
|
||
| class TestTemporaryFileWrapper(BaseTestCase): | ||
| """Test TemporaryFileWrapper.""" | ||
|
|
||
| def test_public_name(self): | ||
| self.assertIs(tempfile.TemporaryFileWrapper, tempfile._TemporaryFileWrapper) | ||
|
|
||
| def test_in_all(self): | ||
| self.assertIn("TemporaryFileWrapper", tempfile.__all__) | ||
|
|
||
| def test_is_return_type_of_named_temporary_file(self): | ||
| with tempfile.NamedTemporaryFile() as f: | ||
|
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. I propose to move this check to any existing test above. |
||
| self.assertIsInstance(f, tempfile.TemporaryFileWrapper) | ||
|
|
||
| class TestSpooledTemporaryFile(BaseTestCase): | ||
| """Test SpooledTemporaryFile().""" | ||
|
|
||
|
|
||
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.
Please, add a comment: why it is there.
Question: does it make sence to deprecate the older alias at the same time?
Uh oh!
There was an error while loading. Please reload this page.
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.
sure 👍
I think we can deprecate in follow up, but if you prefer we should add in this PR, only-- i'm happy to go with
Thanks for the review!
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.
I would deprecate it right away :)