gh-64978: Add `chown()` to `pathlib.Path` by y0urself · Pull Request #31212 · python/cpython

y0urself · 2022-02-08T11:17:24Z

This PR adds chown() and lchown() to the pathlib library.

As for chmod and other existing functions, this is achieved by using os.chown() within the new functions.

Issue: Add pathlib.chown method #64978

the-knights-who-say-ni · 2022-02-08T11:17:27Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@y0urself

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

No support for WindowsPath

JelleZijlstra · 2022-05-02T19:28:11Z

@barneygale what do you think?

barneygale · 2022-05-02T19:43:57Z

I'm supportive of adding chown() - it comes up semi-regularly in devops work.

I don't love lchown() - as you say @JelleZijlstra, it's two ways of spelling the same thing.

Couple more things to consider/discuss:

We already have owner() and group() methods, which return names, not IDs. Would it make sense to accept names here too?
We don't currently expose dir_fd in pathlib, why start here? :)

brettcannon · 2022-05-05T19:53:18Z

I'm fine with adding chown().

I also concur on not wanting lchown().

No opinion on the name/ID question.

I don't quite follow the dir_fd comment as I don't see that in the PR.

bedevere-bot · 2022-05-05T19:54:08Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

y0urself · 2022-05-06T06:11:08Z

I don't quite follow the dir_fd comment as I don't see that in the PR.

I have already removed the dir_fd option.

y0urself · 2022-05-06T06:40:07Z

I am not sure with owner/ group stuff. They return the current owner/group for the given Path object, but these parameters are the ones, we want to change with chown.
If we allow (new_)owner here, we would probably need to go with pwd.getpwnam('new_owner').pw_uid.

On the other hand I have removed lchown() from the PR and I have made the requested changes; please review again.

bedevere-bot · 2022-05-06T06:40:10Z

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

ghost · 2022-05-07T06:28:07Z

All commit authors signed the Contributor License Agreement.

barneygale · 2022-05-07T23:16:23Z

Having given it more thought, and considering the context where this is likely to be used (devops-y scripts), I think supporting owner and group names (in addition to IDs) is important. I think the new method should call through to shutil.chown(), which already accepts int | str | None for each argument. I think the arguments should be spelled owner and group for symmetry with the Path methods. What do you reckon?

Co-authored-by: Barney Gale <barney.gale@gmail.com>

y0urself · 2022-05-09T07:51:29Z

Having given it more thought, and considering the context where this is likely to be used (devops-y scripts), I think supporting owner and group names (in addition to IDs) is important. I think the new method should call through to shutil.chown(), which already accepts int | str | None for each argument. I think the arguments should be spelled owner and group for symmetry with the Path methods. What do you reckon?

We would need to drop the follow_symlinks flag, when we use shutil.chown().

barneygale · 2022-05-09T20:11:25Z

Or add follow_symlinks to shutil.chown()

brettcannon · 2022-05-12T20:58:20Z

+
+ Change the file ownership, like :func:`os.chown`.
+
+ This method normally follows symlinks. Some Unix flavours support changing


Suggested change
This method normally follows symlinks. Some Unix flavours support changing
This method follows symlinks by default. Some Unix flavours support changing

brettcannon · 2022-05-12T20:58:59Z

+ """
+ Change the owner and group id of path to the numeric uid and gid, like os.chown().
+ """


Please follow PEP 8.
Suggested change
"""
Changetheownerandgroupidofpathtothenumericuidandgid, likeos.chown().
"""
"""Change the owner and group id of path to the numeric uid and gid, like os.chown()."""

brettcannon · 2022-05-12T21:01:04Z

+root_in_posix = False
+if hasattr(os, 'geteuid'):
+ root_in_posix = (os.geteuid() == 0)


Suggested change
root_in_posix=False
ifhasattr(os, 'geteuid'):
root_in_posix= (os.geteuid() ==0)
try:
root_in_posix=notos.geteuid()
exceptAttributeError:
root_in_posix=False
This looks to be a copy-and-paste from test_os.py, correct? If so, can you move this to https://github.com/python/cpython/tree/main/Lib/test/support somewhere?

Sure I can move it and use it from there as a helper module ...

brettcannon · 2022-05-12T21:01:51Z

+ @unittest.skipUnless(root_in_posix and len(all_users) > 1,
+ "test needs root privilege and more than one user")
+ def test_chown_with_root(self):
+ # original uid and gid


Please follow PEP 8.
Suggested change
# original uid and gid
# The original uid and gid.

brettcannon · 2022-05-12T21:05:17Z

+try:
+ import pwd
+ all_users = [u.pw_uid for u in pwd.getpwall()]
+except (ImportError, AttributeError):
+ all_users = []


This looks to be a copy-and-paste from test_os.py, correct? If so, can you move this to https://github.com/python/cpython/tree/main/Lib/test/support somewhere?

brettcannon · 2022-05-12T21:05:56Z

+ uid = p.stat().st_uid
+ gid = p.stat().st_gid
+
+ # get users and groups for testing


Suggested change
# get users and groups for testing
# Get users and groups for testing.

bedevere-bot · 2022-05-12T21:06:18Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

brettcannon · 2022-05-19T23:32:49Z

I left a comment on the issue, but after thinking about it, right now there isn't really enough here to warrant taking this PR as it's just wrapping a function call in a method. Without some added benefit to being attached to Path (e.g. higher-level), having to support this API for decades is unfortunately not going to be worth it.

AlexWaygood · 2023-01-08T23:14:22Z

Thanks for the PR, @y0urself. I'm afraid I'm going to close this as rejected, as I agree with Brett's comments, as well as @barneygale's and @CAM-Gerlach's remarks on the associated issue. However, I hope this doesn't discourage you from contributing to CPython in the future! We do appreciate the contribution :)

anki-code · 2023-03-18T13:47:48Z

A few words about one thing.
It will be super cool to have this method because there is awesome python-powered xonsh shell and Path objects are using in xonsh broadly. You can do this in the shell using path-strings:

pipinstall-U'xonsh[full]'xonshcd/tmp&&echoworld>hello.txtp'hello.txt'# Path('hello.txt')p'hello.txt'.read_text()
# 'world\n'forfinp'/tmp'.glob('hello*'):
print(f, f.exists())
# /tmp/hello.txt True

It will be cool to run p'hello.txt'.chown('root') instead of running subprocess chown root hello.txt (it's slower) or do import os etc.

I'm for reopening this issue and favor for enriching pathlib.

CAM-Gerlach · 2023-03-18T14:19:29Z

@barneygale , as you're now officially the pathlib maintainer, what is your take on considering re-opening this PR and/or a revised one?

barneygale · 2023-03-18T15:21:29Z

I've re-opened the issue, we could continue discussion there if that works?

the-knights-who-say-ni added the CLA not signed label Feb 8, 2022

bedevere-bot added the awaiting review label Feb 8, 2022

y0urself force-pushed the fix-issue-20779 branch 2 times, most recently from 6d47346 to 55b70c0Compare February 8, 2022 11:52

y0urself marked this pull request as ready for review February 8, 2022 11:55

ajoino reviewed Feb 8, 2022
View reviewed changes

Comment threadDoc/library/pathlib.rst Outdated

y0urself force-pushed the fix-issue-20779 branch 2 times, most recently from 196cf81 to 9032f1bCompare February 8, 2022 13:47

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Feb 8, 2022

JelleZijlstra reviewed Feb 13, 2022
View reviewed changes

Comment threadDoc/library/pathlib.rst Outdated
Comment threadDoc/library/pathlib.rst Outdated

y0urself added 5 commits February 13, 2022 10:37

bpo-20779 add chown() and lchown() to Path() in pathlib
8906802
No support for WindowsPath

Add tests for chown() and lchown() for pathlib.Path()
5bb6b63

Add patch entry to Misc/NEWS.d/next
dbdbbbe

Add documentation for chown and lchown, fix typo in lchmod
d4070a3

Improve and extend the tests
f9a611b

y0urself force-pushed the fix-issue-20779 branch from c72a2f3 to f9a611bCompare February 13, 2022 09:38

y0urself requested a review from brettcannon as a code owner May 3, 2022 05:44

y0urself force-pushed the fix-issue-20779 branch from ea24cfb to 5e367acCompare May 3, 2022 05:46

brettcannon changed the title ~~bpo-20779: Add chown() and lchown() to pathlib.Path~~gh-64978: Add chown() and lchown() to pathlib.PathMay 5, 2022

brettcannon requested changes May 5, 2022
View reviewed changes

Comment threadDoc/library/pathlib.rst Outdated
Comment threadMisc/NEWS.d/next/Library/2022-02-08-12-26-20.bpo-20779.Gha5Fa.rst Outdated

bedevere-bot removed the awaiting review label May 5, 2022

bedevere-bot added the awaiting changes label May 5, 2022

y0urself changed the title ~~gh-64978: Add chown() and lchown() to pathlib.Path~~gh-64978: Add chown() to pathlib.PathMay 6, 2022

Remove lchown(), don't expose dir_fd
06aebeb

y0urself force-pushed the fix-issue-20779 branch from 5e367ac to 06aebebCompare May 6, 2022 06:17

bedevere-bot added awaiting change review and removed awaiting changes labels May 6, 2022

bedevere-bot requested a review from brettcannon May 6, 2022 06:40

barneygale requested changes May 6, 2022
View reviewed changes

Comment threadMisc/NEWS.d/next/Library/2022-02-08-12-26-20.bpo-20779.Gha5Fa.rst Outdated
Comment threadDoc/library/pathlib.rst

Apply suggestions from code review
cfae2d4
Co-authored-by: Barney Gale <barney.gale@gmail.com>

y0urself force-pushed the fix-issue-20779 branch from 6fa665a to cfae2d4Compare May 10, 2022 06:32

brettcannon self-requested a review May 11, 2022 22:56

brettcannon requested changes May 12, 2022
View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels May 12, 2022

brettcannon added the topic-pathlib label Jun 21, 2022

vajraskymannequin mentioned this pull request Jun 21, 2022
Add pathlib.chown method #64978
Closed

ezio-melotti removed the CLA signed label Jul 13, 2022

AlexWaygood closed this Jan 8, 2023

CAM-Gerlach assigned barneygale Mar 18, 2023

CAM-Gerlach unassigned barneygale Mar 19, 2023


		Change the file ownership, like :func:`os.chown`.

		This method normally follows symlinks. Some Unix flavours support changing

	This method normally follows symlinks. Some Unix flavours support changing
	This method follows symlinks by default. Some Unix flavours support changing

- """
-Changetheownerandgroupidofpathtothenumericuidandgid, likeos.chown().
- """
+"""Change the owner and group id of path to the numeric uid and gid, like os.chown()."""

-root_in_posix=False
-ifhasattr(os, 'geteuid'):
-root_in_posix= (os.geteuid() ==0)
+try:
+root_in_posix=notos.geteuid()
+exceptAttributeError:
+root_in_posix=False

	# get users and groups for testing
	# Get users and groups for testing.

Uh oh!

Uh oh!

Conversation

y0urself commented Feb 8, 2022• edited by AlexWaygood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Feb 8, 2022

CLA Missing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JelleZijlstra commented May 2, 2022

Uh oh!

barneygale commented May 2, 2022

Uh oh!

brettcannon commented May 5, 2022

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented May 5, 2022

Uh oh!

y0urself commented May 6, 2022

Uh oh!

y0urself commented May 6, 2022

Uh oh!

bedevere-bot commented May 6, 2022

Uh oh!

Uh oh!

Uh oh!

ghost commented May 7, 2022• edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barneygale commented May 7, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

y0urself commented May 9, 2022

Uh oh!

barneygale commented May 9, 2022

Uh oh!

brettcannonMay 12, 2022

Choose a reason for hiding this comment

Uh oh!

brettcannonMay 12, 2022

Choose a reason for hiding this comment

Uh oh!

brettcannonMay 12, 2022

Choose a reason for hiding this comment

Uh oh!

y0urselfMay 13, 2022

Choose a reason for hiding this comment

Uh oh!

brettcannonMay 12, 2022

Choose a reason for hiding this comment

Uh oh!

brettcannonMay 12, 2022

Choose a reason for hiding this comment

Uh oh!

brettcannonMay 12, 2022

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 12, 2022

Uh oh!

brettcannon commented May 19, 2022

Uh oh!

AlexWaygood commented Jan 8, 2023

Uh oh!

anki-code commented Mar 18, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CAM-Gerlach commented Mar 18, 2023

Uh oh!

barneygale commented Mar 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

y0urself commented Feb 8, 2022•
edited by AlexWaygood
Loading

ghost commented May 7, 2022•
edited by ghost
Loading

barneygale commented May 7, 2022•
edited
Loading

anki-code commented Mar 18, 2023•
edited
Loading