Next in Review Queue: The New std.path

[Jonathan M Davis]

Names
-----------
dirSeparator:
I'd love to see this as something much shorter like dirSep. I think that 
dirSeparator is overly long - particularly for something which is likely to be 
used primarily in longer expressions.

currentDirSymbol and parentDirSymbol:
The same here. These are _way_ longer than the strings that they represent. If 
we have them, they should be much shorter (e.g. currDirSym and parDirSym). But 
really, I think that I have to concur with dsimcha on this one. The strings 
for these are completely standard, easy to remember, and even shorter names 
for them would still be longer than the actual strings by a fair margin. We 
should just toss them.

isDirSeparator:
I'd love to see this shortened to isDirSep.

extension:
I'd love to see this shortened to ext.

stripExtension:
I'd love to see this shortened to stripExt.

setExtensions:
I think that it should have replace in its name rather than set, since it's 
not altering the original string. It's replacing what's there, not setting it. 
And again, I think that Extension should be  shortened to Ext, so the renamed 
function would be replaceExt.

defaultExtension:
This function is not a property, so it probably shouldn't be a noun, though 
I'm not quite sure what to call it. Technically, it's something like 
setWithDefaultExtensionIfNoExtension, but that's obviously a horrible name 
even if you abbreviate some of it. So, I don't really have a better name, 
though if it's not gonig to be completely renamed, I definitely think that it 
should be shortened to defaultExt rather than having the full Extension in its 
name.

joinPath:
I'm not sure that this function should be renamed to joinPath. When discussing 
other modules such as as std.ascii and std.uni, we decided that they should 
have the same function names and _not_ put the module name in their names. 
Now, this function _is_ joining paths, so it's not purely a case of having 
the module's name in the function name, but I'm still not sure that we want to 
have it as joinPath instead of join.

absolutePath:
It really shouldn't be a noun, since it's not a property. Something like 
makeAbsolute would be better.

Implementation
------------------------
You should probably consider testing the various functions with varying string 
types - both in size and mutability. Just put the test in a foreach loop like

foreach (S; TypeTuple!(string, wstring, dstring))

(but with whatever combination of string types you want to test; In the ideal 
case it would be every possible combination of character type and mutability, 
but that's not necessarily necessary). Then you'll probably end up using 
to!S() on pretty much every string. And for functions which take multiple 
strings, you can test with a combination of string types. It'll make sure that 
the functions work appropriately with the various types. The functions are 
templated, but sometimes something gets missed and they don't work with a 
particular character size or level of mutability. So, while it might be 
overkill to test _every_ combination, verifying at least some of them would be 
desirable.

Personally, I think that it's better practice to test for empty rather than 
length == 0, but I don't suppose that it really matters given that we're 
always dealing with arrays here.

Your note on baseName about it adhering to the requirements for Posix's 
basename utility should probably put in the actual documentation. The same for 
dirName.

There's no point in checking for "== null" like you do in a few places. If you 
want to check whether it's actually null, you need to check for "is null", and 
if you want to check whether it's empty, then you should just use empty. "== 
null" has a tendancy to be misleading, because people tend to think that it 
actually checks whether an array is null, which it doesn't do. Personally, I'm 
likely to think that it's a bug when I see it.

It would probably be more efficient for setExtension to use ~=. e.g.

auto retval = stripExtension(path);
retval ~= '.';
retval ~= ext;
return cast(typeof(return))(retval);

Maybe it doesn't matter (and the code _is_  a bit less clean that way), but it 
might reduce the number of necessary heap allocations. The same goes for 
pretty much anywhere else that ~ is used. It probably doesn't matter all that 
much, but if we want to get the functions as efficient as possible, then it's 
better to use ~= than ~ unless the compiler is actually smart enough to 
tranlate the ~'s into ~='s, but I doubt that it is (particularly since some 
chunk of that is in druntime rather than in the compiler itself).

There's no such thing as an OutOfMemoryException. It's an OutOfMemoryError. 
And being an error, I don't think that there's any need to mention it in the 
documentation. _Anything_ which allocates memory could cause the application 
to run out of memory, and being an Error, you're really not supposed to catch 
it, so there's no real point in mentioning it in the documentation.


Okay. I think that that covers it, though I might have missed something. Aside 
from a few nitpicks, my main issue with the module as it stands is the 
unnecessarily long function names. The function names should be as long as is 
necessary to make them clear but not longer. And putting Extension and 
Separator in names is completely unnecessary. Ext and Sep are quite clear 
enough (especially in context) and are _much_ shorter. The longer names add 
nothing and make the functions/variables much more annoying to deal with in 
longer expressions.

So overall, I think that it looks good and is a definite improvement, but I 
really think that the function names need to be shortened.

- Jonathan M Davis