msg137004 - (view) |
Author: Sandro Tosi (sandro.tosi) *  |
Date: 2011-05-26 20:14 |
Hi, following up the discussion on http://mail.python.org/pipermail/python-dev/2011-May/111642.html here'a patch to introduce shutil.chown() along with doc, tests and a change in os doc to link to the new function (I also took the occasion to rewrap os.chown() doc text). As of now only the one-file function is implemented, let's see at a later time what's about for a recursive one.
The patch was prepared against default, but it probably makes sense to backport it to 2.7?
Any comment/suggestion is welcome!
|
msg137008 - (view) |
Author: Eric V. Smith (eric.smith) *  |
Date: 2011-05-26 20:45 |
As a new feature, it can't be added to 2.7.
|
msg137011 - (view) |
Author: Charles-François Natali (neologix) *  |
Date: 2011-05-26 22:20 |
I didn't test, but after skimming through the code I think that if an invalid user name/group name is provided, os.chown is passed None, which will fail with ValueError.
It would be better to raise an explicit error like "no such user" / "no such group".
That's something you could add to the tests.
Also, you could initialize _user to -1 instead of None, and replace
if user is None:
_user = -1
else:
[...]
with
if user is not None:
[...]
Same goes for _group.
|
msg137084 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-05-27 16:53 |
As a general rule, rewrapping is not done in patches, it can make review less easy. The committer can do it, sometimes in a second commit.
I commented on the review page.
|
msg137174 - (view) |
Author: Gregory P. Smith (gregory.p.smith) *  |
Date: 2011-05-29 09:13 |
I added comments in the code review.
this patch is looking good once the comments are addressed. thanks for your contribution!
As for talk of support for recursion... thats what os.walk() is for. it doesn't belong as part of any particular individual function itself.
|
msg137195 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-05-29 16:31 |
The python-dev discussion talked about chowntree vs. os.walk:
http://mail.python.org/pipermail/python-dev/2011-May/111667.html
http://mail.python.org/pipermail/python-dev/2011-May/111673.html
http://mail.python.org/pipermail/python-dev/2011-May/111674.html
I find those arguments of convenience and precedent convincing.
|
msg137320 - (view) |
Author: Sandro Tosi (sandro.tosi) *  |
Date: 2011-05-30 19:15 |
@Eric: ok, I was just asking given the "unusual" situation 2.7 is (i.e. a very long support series), but it's perfectly fine not to backport the feature.
@Charles-François: I changed a bit the logic: I check for 'None' at first, since it signals that we don't want to change the uid/gid, and then I recheck for None after _get_uid/gid, and only there raise a ValueError (is there a better exception for cases like this?) - what do you think about it?
@Éric: re rewrap: I kinda find that almost every core dev has his opinion on this :) some don't want to rewrap, others are fine if you're touching that paragraph, and so on. Anyhow, to be on the safe-side, I un-rewrapped the first paragraph of os.chown().
@Gregory: I'd like to first concentrate on the "single path" function, then we'll look at the "tree" version.
Thanks a lot to everyone for the review/comment/inspiration: attached you can find the updated patch. If something still needs a fix, I'd be happy to work on it.
|
msg137452 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-06-01 18:04 |
> I was just asking given the "unusual" situation 2.7 is (i.e. a very
> long support series)
Support means bug fixes. Long-term means that the bugfix period (before going into security mode) is extended, not that it can get new features: that’s exclusively for 3.3.
> rewrap: I kinda find that almost every core dev has his opinion on this :)
Heh :)
|
msg138388 - (view) |
Author: Sandro Tosi (sandro.tosi) *  |
Date: 2011-06-15 18:51 |
Here's the updated patch following review on Rietveld
|
msg138671 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2011-06-19 22:57 |
shutil_chown-default-v3.patch:
- os.chown() is only available on Unix, shutil.chown() should not be defined if os.chown() doesn't exist (require to add a @unittest.skipUnless decorator to the test)
- tests: is it possible that shutil.chown() exists whereas pwd or grp module is missing? if yes, you should split the test into two parts. it looks like pwd and grp are avaiable on Unix.
- os.chown(path, -1, -1) is accepted, why not accepting shutil.chown(path) (shutil.chown(path, None, None))?
- style: i don't like _user name, i suggest uid and gid (or user_id and group_id) instead of _user and _group... or you can reuse user/group variables
- style: "else: \n if not isinstance(user, int):" can be written "elif not isinstance(user, int):" to avoid an useless level of indentation
- tests: you may only call os.getuid() and os.getgid() once
|
msg138904 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-06-24 10:51 |
haypo, you missed this comment from gps on Rietveld:
> As for the _user = user stuff, the reason that is being done is so
> that the original user and or group value can be included in the
> raised exception error message. That is actually a nice thing to do.
>
> otherwise, yes, i'd normally just reassign the parameters.
>
> At this point I believe this patch is ready to commit. I'd reword
> the docstring to make it simpler but that is a minor detail and
> something that is easy to change in a future commit.
I agree the patch is ready.
|
msg139603 - (view) |
Author: Sandro Tosi (sandro.tosi) *  |
Date: 2011-07-01 20:52 |
I've applied some of the suggestions Victor made (and also refresh to be cleanly applicable on default).
|
msg140491 - (view) |
Author: Sandro Tosi (sandro.tosi) *  |
Date: 2011-07-16 10:42 |
Sorry for the late reply: I've applied Éric comments made on Rietveld.
|
msg141665 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-08-05 16:25 |
You should replace the v5 file (or even remove all files, for clarity) with the actual output of hg diff, not hg status ;-)
|
msg141686 - (view) |
Author: Sandro Tosi (sandro.tosi) *  |
Date: 2011-08-05 20:36 |
Gaaah, sorry about that: I've just uploaded the correct fifth version.
|
msg141699 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2011-08-06 00:42 |
I wonder if we should raise LookupError for unknown uids/gids. Do we have other APIs with similar semantics?
|
msg141743 - (view) |
Author: Sandro Tosi (sandro.tosi) *  |
Date: 2011-08-07 13:43 |
grepping over the stdlib, LookupError is only used in codecs exception; also the documentation of LE is "The base class for the exceptions that are raised when a key or index used on a mapping or sequence is invalid: IndexError, KeyError. This can be raised directly by codecs.lookup()." so I guess it's not the right exception of this case.
|
msg141749 - (view) |
Author: R. David Murray (r.david.murray) *  |
Date: 2011-08-07 22:12 |
Well, you could consider codecs an example in the stdlib of using it, and the doc could be changed to something like "stdlib modules that look up information in specialized data sources may raise this error directly". Whether this is a good idea or not, I'm not sure, but it feels more logical than ValueError.
|
msg141753 - (view) |
Author: Sandro Tosi (sandro.tosi) *  |
Date: 2011-08-07 22:26 |
On Mon, Aug 8, 2011 at 00:12, R. David Murray <report@bugs.python.org> wrote:
>
> R. David Murray <rdmurray@bitdance.com> added the comment:
>
> Well, you could consider codecs an example in the stdlib of using it, and the doc could be changed to something like "stdlib modules that look up information in specialized data sources may raise this error directly". Whether this is a good idea or not, I'm not sure, but it feels more logical than ValueError.
Sure, I was just showing my line of reasoning :) I'm ok either way: if
you feel LookupError is best, I'll change shutil.chown() code and also
the doc for LE (about that, is it ok a single patch or 2 separate?)
|
msg141784 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-08-08 14:46 |
LookupError sounds good. About the latest patch: I wonder if using !a instead of !r in the format strings for exceptions would be more helpful (maybe you’ve seen a recent python-dev subthread about that). I don’t like seeing escapes for perfectly common characters like ß or é, but OTOH escapes help disambiguating different characters that look the same. Apart from these two points, this is good to go.
A doc addition for LookupError would be an independent changeset; it has nothing to do with adding shutil.chown.
|
msg141792 - (view) |
Author: Sandro Tosi (sandro.tosi) *  |
Date: 2011-08-08 16:26 |
after a review from Ezio (thanks!) we've come out with this updated patch; main changes are in the test suite, where now it's checked that chown() succeed. about !r/!a I've left !r; and changed the 2 ValueError in LookupError (the first, in case no arguments are passed, it's still there).
|
msg142480 - (view) |
Author: Sandro Tosi (sandro.tosi) *  |
Date: 2011-08-19 18:01 |
version 7 here we come :) I've addressed Eric's commenta on rietveld (all but one, I need input from him) and also updated the tests for new helper functions.
|
msg142751 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2011-08-22 21:29 |
New changeset d1fd0f0f8e68 by Sandro Tosi in branch 'default':
#12191: add shutil.chown() to change user and/or group owner of a given path also specifying their names.
http://hg.python.org/cpython/rev/d1fd0f0f8e68
|
msg142752 - (view) |
Author: Sandro Tosi (sandro.tosi) *  |
Date: 2011-08-22 21:30 |
At last, it's in :) thanks a lot to all the people that helped me in the process!
|
msg142753 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2011-08-22 21:33 |
> New changeset d1fd0f0f8e68 by Sandro Tosi in branch 'default'
You may add shutil.chmod to the "What's New in Python 3.3?" document.
|
msg142754 - (view) |
Author: Ezio Melotti (ezio.melotti) *  |
Date: 2011-08-22 21:38 |
Raymond, are you still taking care of the whatsnew?
Do you want people to update it when they add something new?
|
msg142757 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2011-08-22 21:56 |
New changeset 5d317e38da44 by Sandro Tosi in branch 'default':
#12191: fix build failures, explicitly passing group argument when I want to test it
http://hg.python.org/cpython/rev/5d317e38da44
|
msg142764 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-08-22 22:18 |
Ezio: I think that Raymond is in charge of the 3.3 whatsnew too. The rules in a comment at the top of the file still apply: there are already around 30 notes to the file.
|
msg142770 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2011-08-22 22:59 |
New changeset f2cb733c9a37 by Sandro Tosi in branch 'default':
#12191: added entry in What's New (+ small editing on shutil section)
http://hg.python.org/cpython/rev/f2cb733c9a37
|
msg143411 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-09-02 16:15 |
I have added 'chown' in shutil.__all__.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:17 | admin | set | github: 56400 |
2011-09-02 16:15:08 | eric.araujo | set | messages:
+ msg143411 |
2011-08-22 22:59:04 | python-dev | set | messages:
+ msg142770 |
2011-08-22 22:18:42 | eric.araujo | set | messages:
+ msg142764 |
2011-08-22 21:56:11 | python-dev | set | messages:
+ msg142757 |
2011-08-22 21:38:51 | ezio.melotti | set | nosy:
+ rhettinger messages:
+ msg142754
|
2011-08-22 21:33:45 | vstinner | set | messages:
+ msg142753 |
2011-08-22 21:30:00 | sandro.tosi | set | status: open -> closed resolution: fixed messages:
+ msg142752
stage: patch review -> resolved |
2011-08-22 21:29:03 | python-dev | set | nosy:
+ python-dev messages:
+ msg142751
|
2011-08-19 18:01:28 | sandro.tosi | set | files:
+ shutil_chown-default-v7.patch
messages:
+ msg142480 |
2011-08-08 16:26:43 | sandro.tosi | set | files:
+ shutil_chown-default-v6.patch
messages:
+ msg141792 |
2011-08-08 14:46:35 | eric.araujo | set | messages:
+ msg141784 |
2011-08-07 22:26:29 | sandro.tosi | set | messages:
+ msg141753 |
2011-08-07 22:12:34 | r.david.murray | set | nosy:
+ r.david.murray messages:
+ msg141749
|
2011-08-07 13:43:15 | sandro.tosi | set | messages:
+ msg141743 |
2011-08-06 00:42:59 | pitrou | set | nosy:
+ pitrou messages:
+ msg141699
|
2011-08-05 20:36:28 | sandro.tosi | set | nosy:
+ ezio.melotti messages:
+ msg141686
|
2011-08-05 20:35:10 | sandro.tosi | set | files:
- shutil_chown-default-v5.patch |
2011-08-05 20:34:47 | sandro.tosi | set | files:
+ shutil_chown-default-v5.patch |
2011-08-05 16:25:02 | eric.araujo | set | messages:
+ msg141665 |
2011-07-16 10:42:10 | sandro.tosi | set | files:
+ shutil_chown-default-v5.patch
messages:
+ msg140491 |
2011-07-01 20:52:17 | sandro.tosi | set | files:
+ shutil_chown-default-v4.patch
messages:
+ msg139603 |
2011-06-24 10:51:37 | eric.araujo | set | messages:
+ msg138904 |
2011-06-19 22:57:15 | vstinner | set | nosy:
+ vstinner messages:
+ msg138671
|
2011-06-15 18:51:38 | sandro.tosi | set | files:
+ shutil_chown-default-v3.patch
messages:
+ msg138388 |
2011-06-01 18:04:59 | eric.araujo | set | messages:
+ msg137452 |
2011-05-30 19:15:16 | sandro.tosi | set | files:
+ shutil_chown-default-v2.patch
messages:
+ msg137320 |
2011-05-29 16:31:20 | eric.araujo | set | messages:
+ msg137195 |
2011-05-29 09:13:30 | gregory.p.smith | set | nosy:
+ gregory.p.smith messages:
+ msg137174
|
2011-05-27 16:53:31 | eric.araujo | set | nosy:
+ eric.araujo
messages:
+ msg137084 title: Shutil - add chown() in order to allow to use user and group name (and not only uid/gid) -> Add shutil.chown to allow to use user and group name (and not only uid/gid) |
2011-05-27 11:44:36 | giampaolo.rodola | set | nosy:
+ giampaolo.rodola
|
2011-05-27 05:42:27 | petri.lehtinen | set | nosy:
+ petri.lehtinen
|
2011-05-27 05:27:17 | ncoghlan | set | nosy:
+ ncoghlan
|
2011-05-26 22:20:05 | neologix | set | nosy:
+ neologix messages:
+ msg137011
|
2011-05-26 20:45:21 | eric.smith | set | nosy:
+ eric.smith messages:
+ msg137008
|
2011-05-26 20:14:19 | sandro.tosi | set | versions:
+ Python 3.3 |
2011-05-26 20:14:09 | sandro.tosi | create | |