Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ganesh-k13
Copy link
Contributor

@ganesh-k13 ganesh-k13 commented May 14, 2023

Added .path to generate pathlib's Path objects for NamedTemporaryFile and TemporaryDirectory

Note: 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.

@AlexWaygood AlexWaygood added type-feature A feature request or enhancement 3.13 bugs and security fixes labels May 14, 2023
@AlexWaygood AlexWaygood self-requested a review May 14, 2023 06:31
Copy link
Member

@AlexWaygood AlexWaygood left a 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 :)

@AlexWaygood
Copy link
Member

Don't worry about test__xxsubinterpreters crashing; it's happening on main; see #104341

@ganesh-k13
Copy link
Contributor Author

Thanks for the quick review! Yep that makes sense, let me make that change and add to docs 👍

@ganesh-k13
Copy link
Contributor Author

ganesh-k13 commented May 14, 2023

I have pushed changes, comparison: https://github.com/python/cpython/compare/ee6435b52832cb6a102391bfddc92883170249f9..5d6a344082136c396ee3a47068cd047b48c8a59c.

I added a para on namedTemporaryFiles as it is not there today. Can remove/edit if it's not needed.

@barneygale
Copy link
Contributor

barneygale commented May 14, 2023

I've left a comment on the issue about whether we should add a .path attribute or actually inherit from Path. I lean towards the latter at the mo.

@@ -211,6 +214,9 @@ The module defines the following user-callable items:
.. versionchanged:: 3.12
Added the *delete* parameter.

.. versionchanged:: 3.13
Copy link
Contributor

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?

Copy link
Contributor Author

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 👍

Copy link
Contributor Author

@ganesh-k13 ganesh-k13 May 14, 2023

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

(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 :)

@ganesh-k13
Copy link
Contributor Author

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.

@AlexWaygood
Copy link
Member

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

@ganesh-k13
Copy link
Contributor Author

Hey @AlexWaygood / @barneygale, are any more changes needed for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes awaiting review type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants