Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 9, 2019
Merged

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

merged 3 commits into from
Sep 9, 2019

Conversation

@asottile
Copy link
Contributor

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

@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
@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
Lib/test/test_tempfile.py Outdated
Copy link
Member

@tirkarthi tirkarthi Jan 22, 2019

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))?

Copy link
Contributor Author

@asottile asottile Jan 22, 2019

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
Misc/NEWS.d/next/Documentation/2019-01-21-14-30-59.bpo-35803.yae6Lq.rst Outdated
Copy link
Member

@tirkarthi tirkarthi Jan 22, 2019

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.

Copy link
Member

@tirkarthi tirkarthi Jan 22, 2019

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?

Copy link
Contributor Author

@asottile asottile Jan 22, 2019

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

Copy link
Contributor Author

@asottile asottile Jan 22, 2019

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
Copy link
Member

@ammaraskar ammaraskar left a comment

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.

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

@ammaraskar ammaraskar Sep 9, 2019

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

zware
zware reviewed Sep 9, 2019
View reviewed changes
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.
Copy link
Member

@zware zware Sep 9, 2019

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.

Copy link
Contributor Author

@asottile asottile Sep 9, 2019

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
Copy link
Member

@zware zware left a comment

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
Copy link
Contributor

miss-islington commented Sep 9, 2019

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 <[email protected]>
(cherry picked from commit 370138b)

Co-authored-by: Anthony Sottile <[email protected]>
@bedevere-bot
Copy link

bedevere-bot commented Sep 9, 2019

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

@zware
Copy link
Member

zware commented Sep 9, 2019

Thanks for the patch and reviews!

@bedevere-bot
Copy link

bedevere-bot commented Sep 9, 2019

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 <[email protected]>
(cherry picked from commit 370138b)

Co-authored-by: Anthony Sottile <[email protected]>
@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 <[email protected]>
(cherry picked from commit 370138b)

Co-authored-by: Anthony Sottile <[email protected]>
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 <[email protected]>
(cherry picked from commit 370138b)

Co-authored-by: Anthony Sottile <[email protected]>
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 <[email protected]>
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