[Python-Dev] Re: os.path.commonprefix breakage (original) (raw)

Skip Montanaro skip@mojam.com (Skip Montanaro)
Thu, 17 Aug 2000 07:50:28 -0500 (CDT) 

Tim> Oddly enough, I don't: commonprefix worked exactly as documented
Tim> for at least 6 years and 5 months (which is when CVS shows Guido
Tim> checking in ntpath.py with the character-based functionality), and
Tim> got out of synch with the docs about 5 weeks ago when Skip changed
Tim> to this other algorithm.  Since the docs *did* match the code,
Tim> there's no reason to believe the original author was confused, and
Tim> no reason to believe users aren't relying on it (they've had over 6
Tim> years to gripe <wink>).

I don't realize that because a bug wasn't noticed for a long time was any reason not to fix it. Guido was also involved in the repair of the bug, and had no objections to the fix I eventually arrived at. Also, when I announced my original patch the subject of the message was

patch for os.path.commonprefix (changes semantics - pls review)

In the body of the message I said

Since my patch changes the semantics of the function, I submitted a
patch via SF that implements what I believe to be the correct behavior
instead of just checking in the change, so people could comment on it.

I don't think I could have done much more to alert people to the change than I did. I didn't expect the patch to go into 1.6. (Did it? It shouldn't have.) I see nothing wrong with correcting the semantics of a function that is broken when we increment the major version number of the code.

Tim> I appreciate that some other behavior may be more useful more
Tim> often, but if you can ever agree on what that is across platforms,
Tim> it should be spelled via a new function name ("commonpathprefix"
Tim> comes to mind), or optional flag (defaulting to "old behavior") on
Tim> commonprefix (yuck!).  BTW, the presence or absence of a trailing
Tim> path separator makes a *big* difference to many cmds on Windows,
Tim> and you can't tell me nobody isn't currently doing e.g.

Tim>     commonprefix(["blah.o", "blah", "blah.cpp"])

Tim> on Unix either.

Fine. Let's preserve the broken implementation and not break any broken usage. Switch it back then.

Taking a look at the copious documentation for posixpath.commonprefix:

Return the longest string that is a prefix of all strings in
list.  If list is empty, return the empty string ''.

I see no mention of anything in this short bit of documentation taken completely out of context that suggests that posixpath.commonprefix has anything to do with paths, so maybe we should move it to some other module that has no directory path implications. That way nobody can make the mistake of trying to assume it operates on paths. Perhaps string? Oh, that's deprecated. Maybe we should undeprecate it or make commonprefix a string method. Maybe I'll just reopen the patch and assign it to Barry since he's the string methods guru.

On a more realistic note, perhaps I should submit a patch that corrects the documentation.

Skip