Skip to content

bpo-35803: Document and test dir=PathLike for tempfile#11644

Merged
zware merged 3 commits into
python:masterfrom
asottile:dir_pathlike_bpo_35803
Sep 9, 2019
Merged

bpo-35803: Document and test dir=PathLike for tempfile#11644
zware merged 3 commits into
python:masterfrom
asottile:dir_pathlike_bpo_35803

Conversation

@asottile

@asottile asottile commented Jan 21, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Jan 21, 2019
@asottile asottile mentioned this pull request Jan 21, 2019
Closed
tirkarthi
tirkarthi reviewed Jan 22, 2019
View reviewed changes
Comment thread Doc/library/tempfile.rst Outdated
@asottile
asottile force-pushed the dir_pathlike_bpo_35803 branch from 0e73855 to 806270f Compare January 22, 2019 01:07
tirkarthi
tirkarthi reviewed Jan 22, 2019
View reviewed changes
Comment thread Lib/test/test_tempfile.py Outdated

@tirkarthi tirkarthi Jan 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a difference between current code and isinstance(dir, (str, os.PathLike))?

@asottile asottile Jan 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ever so slightly. The current code is "type is exactly str", my patch is "type is exactly str or it implements PathLike", your suggestion is "str or any subclass of str or implements PathLike"

tirkarthi
tirkarthi reviewed Jan 22, 2019
View reviewed changes
Comment thread Misc/NEWS.d/next/Documentation/2019-01-21-14-30-59.bpo-35803.yae6Lq.rst Outdated

@tirkarthi tirkarthi Jan 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit

Suggested change
is a :term:`path-like object`. Patch by Anthony Sottile
is a :term:`path-like object`. Patch by Anthony Sottile.

@tirkarthi tirkarthi Jan 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can state the two functions directly here instead of tempfile functions?

@asottile asottile Jan 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not just two functions, it's every function in the tempfile module

@asottile asottile Jan 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see what you're saying here.

I only added the versionchanged comment to the two functions following the convention for the versionchanged directly above it. The functions in the tempfile module all mention "The dir, prefix and suffix parameters have the same meaning and defaults as with mkstemp()." and don't document those parameters themselves

@asottile
asottile force-pushed the dir_pathlike_bpo_35803 branch from 806270f to 9a15af2 Compare January 22, 2019 01:43
ammaraskar
ammaraskar approved these changes Sep 9, 2019
View reviewed changes

@ammaraskar ammaraskar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with suggestions.

Thanks for the patch Anthony. Confirmed that 3.6 seems to be the first version path objects started working. It would be helpful to introduce this test to ensure that it doesn't accidentally break over time.

Comment thread Lib/test/test_tempfile.py
self.assertIs(
type(name),
str
if type(dir) is str or isinstance(dir, os.PathLike) else

@ammaraskar ammaraskar Sep 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe break this up to it's own line since its getting a bid crowded as a ternary

Comment thread Misc/NEWS.d/next/Documentation/2019-01-21-14-30-59.bpo-35803.yae6Lq.rst Outdated
@asottile @ammaraskar
Update Misc/NEWS.d/next/Documentation/2019-01-21-14-30-59.bpo-35803.y…
eef0e97 
…ae6Lq.rst

Co-Authored-By: Ammar Askar <ammar_askar@hotmail.com>
zware
zware reviewed Sep 9, 2019
View reviewed changes
Comment thread Misc/NEWS.d/next/Documentation/2019-01-21-14-30-59.bpo-35803.yae6Lq.rst Outdated
@@ -0,0 +1,3 @@
Document and test that ``tempfile`` functions may accept a
:term:`path-like object` for the ``dir`` argument. Patch by Anthony Sottile.
is a :term:`path-like object`. Patch by Anthony Sottile.

@zware zware Sep 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggestion went awry here; this last line needs to go away.

@asottile asottile Sep 9, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops! fixed

zware
zware approved these changes Sep 9, 2019
View reviewed changes

@zware zware left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zware
zware merged commit 370138b into python:master Sep 9, 2019
@miss-islington

miss-islington commented Sep 9, 2019

Copy link
Copy Markdown
Contributor

Thanks @asottile for the PR, and @zware for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 9, 2019
@asottile @miss-islington
bpo-35803: Document and test dir=PathLike for tempfile (pythonGH-11644)
6199f1a 
Co-Authored-By: Ammar Askar <ammar_askar@hotmail.com>
(cherry picked from commit 370138b)

Co-authored-by: Anthony Sottile <asottile@umich.edu>
@bedevere-bot

bedevere-bot commented Sep 9, 2019

Copy link
Copy Markdown

GH-15796 is a backport of this pull request to the 3.8 branch.

@zware

zware commented Sep 9, 2019

Copy link
Copy Markdown
Member

Thanks for the patch and reviews!

@bedevere-bot

bedevere-bot commented Sep 9, 2019

Copy link
Copy Markdown

GH-15797 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 9, 2019
@asottile @miss-islington
bpo-35803: Document and test dir=PathLike for tempfile (pythonGH-11644)
2f8fd52 
Co-Authored-By: Ammar Askar <ammar_askar@hotmail.com>
(cherry picked from commit 370138b)

Co-authored-by: Anthony Sottile <asottile@umich.edu>
@asottile
asottile deleted the dir_pathlike_bpo_35803 branch September 9, 2019 15:56
@asottile asottile mentioned this pull request Sep 9, 2019
Merged
miss-islington added a commit that referenced this pull request Sep 9, 2019
@miss-islington @asottile
bpo-35803: Document and test dir=PathLike for tempfile (GH-11644)
eadf6b8 
Co-Authored-By: Ammar Askar <ammar_askar@hotmail.com>
(cherry picked from commit 370138b)

Co-authored-by: Anthony Sottile <asottile@umich.edu>
miss-islington added a commit that referenced this pull request Sep 9, 2019
@miss-islington @asottile
bpo-35803: Document and test dir=PathLike for tempfile (GH-11644)
b4591ad 
Co-Authored-By: Ammar Askar <ammar_askar@hotmail.com>
(cherry picked from commit 370138b)

Co-authored-by: Anthony Sottile <asottile@umich.edu>
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@asottile @ammaraskar @websurfer5
bpo-35803: Document and test dir=PathLike for tempfile (pythonGH-11644)
208b0de 
Co-Authored-By: Ammar Askar <ammar_askar@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tirkarthi tirkarthi tirkarthi left review comments
@ammaraskar ammaraskar ammaraskar approved these changes
@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@zware zware zware approved these changes

Assignees

No one assigned

Labels

tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@asottile @miss-islington @bedevere-bot @zware @ammaraskar @JelleZijlstra @tirkarthi @the-knights-who-say-ni