-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
gh-87646: Added .path
to tempfile
#104466
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?
Conversation
Misc/NEWS.d/next/Library/2023-05-14-11-46-21.gh-issue-87646.DNJjDX.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Library/2023-05-14-11-46-21.gh-issue-87646.DNJjDX.rst
Outdated
Show resolved
Hide resolved
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 like the idea; I'm always doing stuff like:
with TemporaryDirectory() as td:
td_path = Path(td)
And it feels annoying!
I'd prefer it if we used functools.cached_property
— I think it looks nicer as an attribute, but I think lazy evaluation is preferable here, or we'll be slowing down TemporaryDirectory() calls for users who aren't interested in this feature.
Also, this will need documentation to be added to Doc/library/tempfile.rst
:)
Don't worry about |
Thanks for the quick review! Yep that makes sense, let me make that change and add to docs 👍 |
I have pushed changes, comparison: https://github.com/python/cpython/compare/ee6435b52832cb6a102391bfddc92883170249f9..5d6a344082136c396ee3a47068cd047b48c8a59c. I added a para on |
I've left a comment on the issue about whether we should add a |
@@ -211,6 +214,9 @@ The module defines the following user-callable items: | |||
.. versionchanged:: 3.12 | |||
Added the *delete* parameter. | |||
|
|||
.. versionchanged:: 3.13 |
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.
Shouldn't new path property be explained somewhere?
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 thought same, but even the .name
is not explained in this file. I can add it if someone can point an apt location 👍
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.
NamedTemporaryFile
indeed had a reference to .name
, I added .path
there. Thanks!, ref: diff. I'll leave the TemporaryDirectory
for now, we may need to add name
and path
Added `.path` property to generate pathlib's `Path` objects for NamedTemporaryFile and TemporaryDirectory
(Nit: where possible, please avoid force pushes when contributing to CPython. They interact badly with the GitHub UI, making it hard to see what changed between commits. All PRs are squashed into a single commit when they are merged, anyway, so there's no need to worry about having a messy commit history on a PR :) |
Ah ok. I wanted to know what was the convention here. Thanks a lot for letting me know! I'll follow this from now on. |
No need to read it all, but when in doubt, the CPython devguide has lots of information about CPython's specific workflow, which can be useful in these situations :) https://devguide.python.org |
Hey @AlexWaygood / @barneygale, are any more changes needed for this PR? |
Added
.path
to generate pathlib'sPath
objects for NamedTemporaryFile and TemporaryDirectoryNote: Original suggestion was to make it a method so the
Path
creation will be lazy. I'm ok with both and wanted the opinion of a maintainer.