Std.path: Final Vote

[Andrei Alexandrescu]

On 8/5/11 7:26 AM, dsimcha wrote:
> My apologies for not announcing this yesterday. For some reason I
> thought today was the official end of review. Anyhow, Lars Kyllingstad's
> new std.path module, which has been in review for the past 3 weeks, is
> up for vote. Please vote yes or no in this thread.
>
> Code:
> https://github.com/kyllingstad/phobos/blob/std-path/std/path.d
>
> Docs
> http://www.kyllingen.net/code/std-path/phobos-prerelease/std_path.html
>
> Voting ends next Friday, August 12, since it started a day late.

My vote is "yes", with a few advisory comments.

Comments on the documentation:

* "This module is used to parse path strings." -> "This module is used 
to manipulate path strings."

* "perform any I/O" -> "perform any actual file system actions"

* "use std.file.isDir and std.file.exists" -> use the XREF macro to 
generate cross-reference links.

* "backslashes on this platform" -> "backslashes on that platform"

* "The result of calling a function on an ill-formed path is undefined." 
This simplifies documentation but is a bit extreme. We could and should 
specify the behavior for strings that don't look quite like paths.

* BTW we should have a validPath() function that tells whether a string 
looks like a valid path or not.

* Add example:

assert (baseName("dir/file.ext", ".xyz") == "file.ext");

* defaultExtension -> setDefaultExtension?

* absolutePath calls getcwd(), negating the assertion in the beginning 
that there's no real directory access going on.

* absolutePath and others use string, others use generic characters. 
Why? (This is my strongest comment.)

* filenameCharCmp and filenameCmp -> why long and not int?

* Example in expandTilde uses odd ALL_UPERCASE variable names.

Comments on the implementation:

* We're increasingly moving towards consolidating imports into one.

* "." is hardcoded as a symbol for the current dir.

* Would be interesting to figure what it would take to make pathSplitter 
reuse splitter.

* Misalignment in lines 2195--2238. I think you don't need the extra 
scope there anyway, but if you do, don't make a special rule for that 
case - obey normal brace indentation.


Andrei