[proxy] web.archive.org — ← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
/ cpython Public
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

Jump to bottom

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

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

Copy link
Contributor

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

bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Jan 21, 2019
asottile mentioned this pull request Jan 21, 2019
Closed
tirkarthi reviewed Jan 22, 2019
View changes
Doc/library/tempfile.rst Outdated Show resolved Hide resolved
asottile force-pushed the dir_pathlike_bpo_35803 branch from 0e73855 to 806270f Compare Jan 22, 2019
tirkarthi reviewed Jan 22, 2019
View changes
Lib/test/test_tempfile.py
self.assertIs(
type(name),
str
if type(dir) is str or isinstance(dir, os.PathLike) else
Copy link
Member

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 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 reviewed Jan 22, 2019
View changes
Misc/NEWS.d/next/Documentation/2019-01-21-14-30-59.bpo-35803.yae6Lq.rst Outdated
@@ -0,0 +1,2 @@
Document and test that ``tempfile`` functions may take a ``dir`` which
is a :term:`path-like object`. Patch by Anthony Sottile
Copy link
Member

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 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 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 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 force-pushed the dir_pathlike_bpo_35803 branch from 806270f to 9a15af2 Compare Jan 22, 2019
ammaraskar approved these changes Sep 9, 2019
View changes
Copy link
Member

ammaraskar left a comment

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

Misc/NEWS.d/next/Documentation/2019-01-21-14-30-59.bpo-35803.yae6Lq.rst Outdated Show resolved Hide resolved
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 reviewed Sep 9, 2019
View 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 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 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 approved these changes Sep 9, 2019
View changes
Copy link
Member

zware left a comment

LGTM

zware merged commit 370138b into python:master Sep 9, 2019
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
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>
Copy link

bedevere-bot commented Sep 9, 2019

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

Copy link
Member

zware commented Sep 9, 2019

Thanks for the patch and reviews!

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
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 deleted the dir_pathlike_bpo_35803 branch Sep 9, 2019
asottile mentioned this pull request Sep 9, 2019
Merged
miss-islington added a commit that referenced this pull request Sep 9, 2019
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
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
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

ammaraskar

JelleZijlstra

zware

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.

None yet
8 participants