classification
Title: followlinks/follow_symlinks/symlinks flags unification
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, eric.araujo, georg.brandl, hynek, larry, pitrou, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2012-06-27 08:59 by serhiy.storchaka, last changed 2012-08-05 10:43 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
followlinks.patch serhiy.storchaka, 2012-06-27 08:59 Patch for renaming "follow_symlinks" to "followlinks" review
followlinks-to-follow_symlinks.patch serhiy.storchaka, 2012-06-27 18:36 Patch for renaming "followlinks" to "follow_symlinks" review
symlinks-to-follow_symlinks.patch serhiy.storchaka, 2012-06-27 21:05 review
followlinks-to-follow_symlinks-3.patch serhiy.storchaka, 2012-07-01 13:10 Patch for renaming "followlinks" in new "fwalk" function to "follow_symlinks" review
symlinks-to-follow_symlinks-3.patch serhiy.storchaka, 2012-07-01 13:10 Patch for renaming new "symlinks" parameter in shutil functions to "follow_symlinks" review
followlinks-to-follow_symlinks-4.patch serhiy.storchaka, 2012-07-02 15:16 review
symlinks-to-follow_symlinks-4.patch serhiy.storchaka, 2012-07-02 15:43 review
Messages (29)
msg164130 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-06-27 08:59
Now three different flag are used to denote the same behavior. followlinks is used in os.(f)walk, symlinks is used in shutil functions (with opposite meaning), and follow_symlinks is just added to many os functions. Unfortunately, symlinks, like followlinks, is used prior to 3.3. But follow_symlinks appeared only in 3.3 (besides, it's too long a name for this frequently used parameter) and it can be unified with one of the earlier spelling. Replacing follow_symlinks to followlinks is simpler than to symlinks.
msg164134 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-06-27 09:35
Yeah, would be nice if that was consistent.
msg164136 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-06-27 09:52
I assert that "followlinks" and "symlinks" are both bad names.  I dislike "followlinks" because "links" is ambiguous; both hard links and soft links are "links", but it's only modifying behavior regarding one of them.  Also, it's not PEP-8-compliant (which we can forgive because I'm pretty sure it predates PEP 8).  "symlinks" is far worse, because it's so ambiguous--quick, what does "symlinks=False" mean?  Examine symlinks, or follow them?

I agree that we can't rename "followlinks" and "symlinks" in 3.x.  All we can do for now is move forward.  At the same time I refused to be shackled by misguided old nomenclature.

So, certainly, I don't want to see "follow_symlinks" changed.  True story: the reason I started writing the patch for #14626 was so I could make sure it used the name "follow_symlinks".  I was dead certain Serhiy would use one of the existing names ;-)


If you really really want this to happen, you'll have to get Georg's permission--and the sooner the better.  Already I suspect it is too late.  If it ships in 3.3 it will absolutely be too late.


I suggest another approach: add a redundant "follow_symlinks" argument to os.walk, os.fwalk, and the shutil functions.  Prefer the new name in documentation but document the presence of the old one.  Throw an exception if both are specified in an invocation.  We could do that in 3.4.
msg164146 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-06-27 11:12
> Also, it's not PEP-8-compliant (which we can forgive because I'm pretty sure it predates PEP 8).

I don't see how this is contrary to PEP 8. PEP 8 says nothing about the
names of function arguments.

> Already I suspect it is too late.  If it ships in 3.3 it will
> absolutely be too late.

Unfortunately, too little time has passed between #14626 accepting and
shipping of 3.3b1. In any case, with accepted #14626 better than without
it.

> I suggest another approach: add a redundant "follow_symlinks" argument
> to os.walk, os.fwalk, and the shutil functions.

This is a bad solution, and I do not think that the benefits would
outweigh the disadvantages. I am -0.
msg164147 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-06-27 11:55
> I don't see how this is contrary to PEP 8. PEP 8 says nothing about
> the names of function arguments.

Certainly it says *something*:

http://www.python.org/dev/peps/pep-0008/#function-and-method-arguments

But I grant you, it only specifically addresses "self", "cls", and function argument names that clash with keywords.

I therefore suggest that function arguments are most similar to "method names and instance variables"--after all, they *are* instance variables.  And "method names and instance variables" say "Use the function naming rules".  And function names "should be lowercase, with words separated by underscores as necessary to improve readability."

"followlinks" is comprised of two words but they are not separated by underscores.

(We can also reason by process of elimination: function arguments are wholly dissimilar to constants, package/module names, classes, and exception.  All the remaining types of identifiers in Python follow the above rule.)

It might actually be nice to clarify PEP 8 on this.


> Unfortunately, too little time has passed between #14626 accepting
> and shipping of 3.3b1.

I am genuinely sorry about that--but #14626 just wasn't ready earlier.  I'm glad you think it's an improvement--we can certainly agree on that!
msg164151 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-06-27 13:24
Although I do dislike how long it is, I think I agree with larry on this one that follow_symlinks is the cleanest choice.  And we are post feature-freeze, so I think changing it should be done only if there is a strong reason.
msg164166 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-06-27 15:23
IMO adding follow_symlinks to the functions currently supporting symlinks/followlinks is a bugfix.  Such a patch would be ok to go into 3.3.
msg164173 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-06-27 17:02
> I dislike "followlinks" because "links" is ambiguous; both hard links
> and soft links are "links", but it's only modifying behavior regarding
> one of them.

Technically, in Unix world any file is a hard link. It is impossible to
distinguish a hard link from the "original", they are completely equal.
So I don't think that there will be a ambiguity, what links are implied,
especially, if the documentation is clearly says "symbolic links".

> I therefore suggest that function arguments are most similar to "method names and instance variables"--after all, they *are* instance variables.

Technically, they are local variables.

> "followlinks" is comprised of two words but they are not separated by underscores.

As "getgrouplist" or "sendfile". What about such argument names as
"filename" and "newline"? If being a consistent in that, then it must be
"follow_symbolic_links". And "source_directory_file_descriptor" instead
"src_dir_fd".

> It might actually be nice to clarify PEP 8 on this.

I agree.

> I'm glad you think it's an improvement--we can certainly agree on that!

Of course. This is the issue, for the solution of which I registered
here. All other issues and patches were just distractions and
practice. ;-)  I am grateful to you.
msg164179 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-06-27 17:57
> IMO adding follow_symlinks to the functions currently supporting symlinks/followlinks is a bugfix.  Such a patch would be ok to go into 3.3.

Here is an other patch, that implements Larry's suggestion about
renaming followlinks in (f)walk to follow_symlinks (with keeping old
name as alias). I hope native speakers corrected me in docs. However, I
don't think that this is the best solution (but it better than nothing).
msg164195 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-06-27 20:29
> > after all, they *are* instance variables.
>  Technically, they are local variables.

Yeah, tbh I was thinking "instance of an invocation" here.  But PEP 8 probably means "instance of a class" here.

Still, there are three classes of naming things in Python: all uppercase (constants), CapCase (class and exception names), and everything else (lowercase with underscores).  I suggest that arguments and local variables belong in the latter camp.  At no point does PEP 8 suggest naming anything in lowercase without underscores.

> As "getgrouplist" or "sendfile". What about such argument names as
> "filename" and "newline"? If being a consistent in that, then it
> must be "follow_symbolic_links". And
> "source_directory_file_descriptor" instead "src_dir_fd".

You are muddling the issue--PEP 8 makes no ruling on abbreviations.

Personally, I prefer not to use abbreviations where possible.  I've found over the years that they are painfully ambiguous--I can never remember what abbreviation I used.  ("control" becomes... "ctl"? "cntl"?)  MvL also makes a good point that abbreviations are a hinderance to people who speak little or no English.

That said, naming things after their POSIX counterparts has to be okay, and abbreviations are occasionally called for when they are widely understood / have a precedent in the library.  So with every example you cite I prefer the extant name over any counterproposal you explicitly suggested.
msg164200 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-06-27 21:05
And here is a patch, that replaces "symlinks" arguments in shutil by
"follow_symlinks" (with keeping old name if it exists before 3.3). I
very hope native speakers corrected me in docs.
msg164227 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-06-28 08:25
Here is updated patches. The type of exception thrown when specifying
mutually exclusive arguments changed from ValueError to TypeError (in
accordance with the raised in similar conflicts exceptions). Slightly
modified documentation. Taken into account some of Larry's comments.

Only os.walk had followlinks and shutil.copytree had symlinks before
3.3. So these are the only functions preserved the old arguments.
os.fwalk and another five public functions from shutil got a new
argument only in 3.3, for them it just renamed (may be to make them
keyword-only?). Larry worrying, isn't it too late? But it would be
strange to have in 3.3 new and immediately obsolete arguments. Moreover
the support for the two arguments complicate and slow down the code. If
you postpone it to 3.4, there will not be another solution, but now we
can do less mess.
msg164452 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-01 07:11
storchaka: I can (finally!) spend some time reviewing patches today.  Which ones do I look at?  I'm guessing just the last two, "followlinks-to-follow_symlinks-2.patch" and "symlinks-to-follow_symlinks-2.patch".  But I wanted to confirm before I got knee-deep in it.
msg164464 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-01 09:20
Bad news: Fearless Leader (Georg) just told me on #python-dev that he's had a change of heart and doesn't want this in 3.3.  I've marked the bug 3.4, and there's no rush on doing this work--we can't check it in until the 3.4 branch exists.  Sorry, Serhiy :(
msg164465 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-01 09:24
Georg just clarified: we can just change the parameter names for new APIs.  It's the deprecation / new parameter stuff we can't do for 3.3.  So cut a (much) simpler patch and let's get it in right quick.
msg164478 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-07-01 13:10
Here is an updated patches (with index 3). No aliased parameters, only
new parameters renamed. Some minor errors have fixed.
msg164507 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-02 06:11
Serihy: for the "followlinks" patch, how about we plan ahead: I give you feedback for just the code (if there is any), and then I take over the patch and do the doc rewrite that I will find irresistable.
msg164515 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-02 14:10
Storchaka: change of plan.  Since doc changes are much lower risk than code changes, how about we go with your existing patch and I'll fix up the docs separately.

So, please make the relevant code changes I proposed on Rietveld (adding "*," everywhere was it iirc) and resubmit and we can get that in.  I'll give you a code-only review of the other patch too--I'll start on that now.
msg164518 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-07-02 15:16
> So, please make the relevant code changes I proposed on Rietveld (adding "*," everywhere was it iirc) and resubmit and we can get that in.  I'll give you a code-only review of the other patch too--I'll start on that now.

Here is a changed patches.

Personally, I doubt that followlinks-to-follow_symlinks-4.patch is
better than followlinks-to-follow_symlinks-3.patch. Also my concern is
the incompatibility of the arguments of os.walk and os.fwalk.
msg164519 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-02 15:20
What's the urge to make parameters keyword-only?
Also, copyfile() shouldn't change signature.
msg164520 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-07-02 15:49
> What's the urge to make parameters keyword-only?

If anyone uses these functions with a new "symlinks" parameter in 3.3b1,
he will get loud error (follow_symlinks == not symlinks).

> Also, copyfile() shouldn't change signature.

Why not?
msg164521 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-02 15:52
> > Also, copyfile() shouldn't change signature.

> Why not?

Seems like I have misread the docs. Apparently the symlinks parameter was added in 3.3.
msg164575 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-03 05:27
> What's the urge to make parameters keyword-only?

I suggest that boolean parameters are best made keyword-only.  Otherwise you have mystery meat like

shutil.copyfile("src", "dst", True)

Also, all the extant uses of "follow_symlinks" in os are keyword-only, and as long as it's a new parameter it'd be nice to keep it consistent.
msg165485 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-15 01:28
The .4 patches both LGTM, please commit!
msg165536 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-15 16:58
> The .4 patches both LGTM, please commit!

You're the one with commit rights here ;)
msg165542 - (view) Author: Roundup Robot (python-dev) Date: 2012-07-15 17:58
New changeset 758a9023d836 by Larry Hastings in branch 'default':
Issue #15202: Consistently use the name "follow_symlinks" for
http://hg.python.org/cpython/rev/758a9023d836
msg165548 - (view) Author: √Čric Araujo (eric.araujo) * (Python committer) Date: 2012-07-15 20:29
There are versionadded notes in the doc that still use the old names (e.g. symlinks for shutil.copyfile).
msg165554 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-15 23:24
Sorry; the patch didn't apply cleanly, and it looks like I bungled doing it manually.  Fixing now.
msg165555 - (view) Author: Roundup Robot (python-dev) Date: 2012-07-15 23:59
New changeset e26113f17309 by Larry Hastings in branch 'default':
Issue #15202: Additional documentation fixes inadvertently omitted
http://hg.python.org/cpython/rev/e26113f17309
History
Date User Action Args
2012-08-05 10:43:37serhiy.storchakasetstatus: open -> closed
resolution: fixed
2012-07-15 23:59:09python-devsetmessages: + msg165555
2012-07-15 23:24:21larrysetmessages: + msg165554
2012-07-15 20:29:41eric.araujosetnosy: + eric.araujo
messages: + msg165548
2012-07-15 17:58:08python-devsetnosy: + python-dev
messages: + msg165542
2012-07-15 16:58:38pitrousetmessages: + msg165536
2012-07-15 01:28:53larrysetmessages: + msg165485
2012-07-03 05:27:08larrysetmessages: + msg164575
2012-07-02 15:52:46pitrousetmessages: + msg164521
2012-07-02 15:49:33serhiy.storchakasetmessages: + msg164520
2012-07-02 15:43:45serhiy.storchakasetfiles: + symlinks-to-follow_symlinks-4.patch
2012-07-02 15:38:51serhiy.storchakasetfiles: - symlinks-to-follow_symlinks-4.patch
2012-07-02 15:20:48pitrousetnosy: + pitrou
messages: + msg164519
2012-07-02 15:16:24serhiy.storchakasetfiles: + followlinks-to-follow_symlinks-4.patch, symlinks-to-follow_symlinks-4.patch

messages: + msg164518
2012-07-02 14:10:11larrysetmessages: + msg164515
2012-07-02 06:11:14larrysetmessages: + msg164507
2012-07-01 13:10:38serhiy.storchakasetfiles: - symlinks-to-follow_symlinks-2.patch
2012-07-01 13:10:17serhiy.storchakasetfiles: - followlinks-to-follow_symlinks-2.patch
2012-07-01 13:10:09serhiy.storchakasetfiles: + followlinks-to-follow_symlinks-3.patch, symlinks-to-follow_symlinks-3.patch

messages: + msg164478
2012-07-01 09:24:15larrysetmessages: + msg164465
versions: + Python 3.3, - Python 3.4
2012-07-01 09:20:25larrysetmessages: + msg164464
versions: + Python 3.4, - Python 3.3
2012-07-01 07:11:42larrysetmessages: + msg164452
2012-06-28 08:25:11serhiy.storchakasetfiles: + followlinks-to-follow_symlinks-2.patch, symlinks-to-follow_symlinks-2.patch

messages: + msg164227
2012-06-27 21:05:52serhiy.storchakasetfiles: + symlinks-to-follow_symlinks.patch

messages: + msg164200
2012-06-27 20:29:43larrysetmessages: + msg164195
2012-06-27 18:36:11serhiy.storchakasetfiles: + followlinks-to-follow_symlinks.patch
2012-06-27 18:34:52serhiy.storchakasetfiles: - followlinks-to-follow_symlinks.patch
2012-06-27 17:57:44serhiy.storchakasetfiles: + followlinks-to-follow_symlinks.patch

messages: + msg164179
2012-06-27 17:02:57serhiy.storchakasetmessages: + msg164173
2012-06-27 15:50:39Arfreversetnosy: + Arfrever
2012-06-27 15:23:31georg.brandlsetmessages: + msg164166
2012-06-27 13:24:21r.david.murraysetnosy: + r.david.murray
messages: + msg164151
2012-06-27 11:55:58larrysetmessages: + msg164147
2012-06-27 11:12:32serhiy.storchakasetmessages: + msg164146
2012-06-27 09:52:18larrysetnosy: + georg.brandl
messages: + msg164136
2012-06-27 09:50:19serhiy.storchakasettype: behavior
versions: + Python 3.3
2012-06-27 09:35:19hyneksetnosy: + hynek
messages: + msg164134
2012-06-27 09:34:07serhiy.storchakasetnosy: + larry
2012-06-27 09:00:10serhiy.storchakasettitle: followlinks/follow_symlinks/symlinks flags unification. -> followlinks/follow_symlinks/symlinks flags unification
2012-06-27 08:59:38serhiy.storchakacreate