bpo-29447: tempfile: Add pathlike object support for dir argument by louisom · Pull Request #1496 · python/cpython

louisom · 2017-05-08T03:26:51Z

No description provided.

mention-bot · 2017-05-08T03:26:55Z

@lulouie, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ezio-melotti, @birkenfeld and @serhiy-storchaka to be potential reviewers.

brettcannon

Don't forget to add yourself to Misc/ACKS.

brettcannon · 2017-05-08T21:49:27Z


      The :py:data:`os.O_TMPFILE` flag is now used if available.

+   .. versionchanged:: 3.7


So this isn't entirely true since you didn't need to change anything for support, so this really has been supported since Python 3.6.

brettcannon · 2017-05-08T21:49:37Z


+   .. versionchanged:: 3.7
+
+      The ``dir`` argument is now accept a :term:`path-like object`.


The *dir* argument now accepts a :term:`path-like object`.

I think no documentation changes is needed. Otherwise we should add "versionchanged" directives for every function that got an implicit support of path-like objects (this is perhaps many tens of functions). This just distracts the attention.

Revert the docs change for now.

brettcannon · 2017-05-08T21:53:45Z

+    def test_choose_pathlike_directory(self):
+        # _mkstemp_inner can create files in a user-selected pathlike directory
+        dir = pathlib.Path(tempfile.mkdtemp())
+        try:


Instead of a try/finally, use unittest.TestCase.addCleanup() (basically you shouldn't have to use any try/finally blocks in your test code thanks to that method).

brettcannon · 2017-05-08T21:55:26Z

 - bpo-30068: _io._IOBase.readlines will check if it's closed first when
  hint is present.

+- bpo-29447: tempfile's dir argument is now accept path-like objects.


This isn't accurate since the support was already there implicitly, so just drop the changes to this file or add a comment that tests for path-like objects in tempfile were added to the "Tests" section.

Usually we don't add entries about changes to tests unless this is very large change or a change that affects the testing infrastructure (test.support, test.libregrtest).

Remove the NEWS entry.

brettcannon · 2017-05-08T21:56:06Z

 class TestLowLevelInternals(unittest.TestCase):
    def test_infer_return_type_singles(self):
        self.assertIs(str, tempfile._infer_return_type(''))
+        self.assertIs(str, tempfile._infer_return_type(pathlib.Path('')))


It wouldn't hurt to add a test for a path-like object that returns bytes (you will have to make a test object yourself).

There are test path-like classes in other test files. The code can be shared.

I couldn't find other place test path-like with bytes. Add some test here, and at low-level internals test

Search def __fspath__ in tests.

brettcannon · 2017-05-08T21:57:06Z

        self.do_create(dir=".")
+        self.do_create(dir=pathlib.Path("."))

    def test_basic_with_bytes_names(self):


Add a test with a path-like object that returns bytes.

brettcannon

If those changes you made to tempfile are necessary then the doc changes you reverted will need to come back as this is no longer a patch to just add some tests.

brettcannon · 2017-05-09T19:06:43Z

    return_type = None
    for arg in args:
-        if arg is None:
+        if arg is None or isinstance(arg, _os.PathLike):


Why is this necessary? Can't you simply call _os.fspath(arg) just before if isinstance(arg, bytes)?

I think dir = _os.fspath(dir) should be called before calling _infer_return_type(). It would be not correct to call _os.fspath() for non-path arguments.

@serhiy-storchaka sounds reasonable to me.

We can't do dir = _os.fspath(dir) too fast, otherwise path-like objects will only be treat as an str, not str or bytes.

os.fspath() returns str or bytes.

ah, I get it. Is my misunderstand about path-like objects, I thought path-like object should return string only. because stdlib implement __fspath__ with return str only. And there should be some 3rd-party class implement __fspath__ with return bytes.

For example DirEntry.__fspath__ can return bytes.

brettcannon · 2017-05-09T19:09:22Z

            dir = gettempdir()
        else:
            dir = gettempdirb()
+    if isinstance(dir, _os.PathLike):


Why is the encoding call suddenly necessary when it wasn't needed for dir previously?

We should fist checkout the output type, to respect other arguments (prefix and suffix), then decide how to convert dir when it is a path-like objects.

serhiy-storchaka · 2017-05-10T10:02:36Z

    for arg in args:
        if arg is None:
            continue
+        elif isinstance(arg, _os.PathLike):


Why this explicit test is needed? Aren't high-level function fail in any case when pass path-like object as prefix or suffix?

Should we allow path-like objects for prefix and suffix? it will failed when path-like objects concat with string:
file = _os.path.join(dir, pre + name + suf)

@lulouie No, path-like objects are only useful for things that represent paths. Both prefix and suffix are not paths themselves.

brettcannon · 2018-02-02T22:04:58Z

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

serhiy-storchaka

Path-like objects shouldn't be accepted as prefix or suffix. There should be tests for this.

serhiy-storchaka · 2018-03-26T14:26:03Z

    TEST_FILES = 100

+
+class _PathLikeObj(object):


Use test.support.FakePath.

csabella · 2019-05-31T13:02:32Z

As this pull request is from an 'unknown repository', I'm going to close it. A new pull request can be opened with this change from a valid repository.

Add pathlike object support

0c0d7df

the-knights-who-say-ni added the CLA signed label May 8, 2017

Fix path value

4ab46fe

louisom changed the title ~~Add pathlike object support~~ bpo-29947: Add pathlike object support May 8, 2017

louisom changed the title ~~bpo-29947: Add pathlike object support~~ bpo-29447: Add pathlike object support May 8, 2017

Fix doc

d2e1168

brettcannon requested changes May 8, 2017

View reviewed changes

louisom added 5 commits May 9, 2017 14:33

Revert tempfile doc

f16b99c

Fix low-level internals support path-like with bytes

9afde67

Add and fix test case

e9b7b2e

Remove NEWS entry

21871d0

Add Louie Lu to ACKS

9345faf

brettcannon requested changes May 9, 2017

View reviewed changes

louisom added 2 commits May 10, 2017 17:51

Fix low-level internal for path-like objects

e670c71

Fix path-like test

949f9fd

serhiy-storchaka reviewed May 10, 2017

View reviewed changes

Fix dir argument

732bbf4

louisom changed the title ~~bpo-29447: Add pathlike object support~~ bpo-29447: tempfile: Add pathlike object support for dir argument May 10, 2017

brettcannon added the awaiting changes label Feb 2, 2018

serhiy-storchaka requested changes Mar 26, 2018

View reviewed changes

csabella closed this May 31, 2019

brettcannon mentioned this pull request Apr 10, 2022

Add/check os.PathLike support for the tempfile module's 'dir' arguments #73633

Closed


		The :py:data:`os.O_TMPFILE` flag is now used if available.

		.. versionchanged:: 3.7


		.. versionchanged:: 3.7

		The ``dir`` argument is now accept a :term:`path-like object`.

Uh oh!

Conversation

louisom commented May 8, 2017

Uh oh!

mention-bot commented May 8, 2017

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisom May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettcannon commented Feb 2, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csabella commented May 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

louisom May 10, 2017 •

edited

Loading