code review 5503067: path/filepath: Dir

path/filepath: Dir

There was Base but not Dir, so fill in the gap.

[r, 2011-12-22 18:17:14]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[niemeyer, 2011-12-22 18:21:01]

LGTM. Besides being Base's complement, makes manipulation slightly simpler in
some cases by enabling calls like f(filepath.Dir(path)).

Should the path package receive the counterpart too?

[niemeyer, 2011-12-22 18:27:23]

http://codereview.appspot.com/5503067/diff/1/src/pkg/path/filepath/path_test.go
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/5503067/diff/1/src/pkg/path/filepath/path_test....
src/pkg/path/filepath/path_test.go:442: {"abc/def", "abc"},
Hmm.. I do wonder about this case, on a second look. Should this have a trailing
slash, to match Split's behavior? It may be slightly surprising that the result
of Split(path) and Dir(path) yield different dir values.

[r2, 2011-12-22 18:31:21]

On Dec 22, 2011, at 10:27 AM, n13m3y3r@gmail.com wrote:

> 
>
http://codereview.appspot.com/5503067/diff/1/src/pkg/path/filepath/path_test.go
> File src/pkg/path/filepath/path_test.go (right):
> 
>
http://codereview.appspot.com/5503067/diff/1/src/pkg/path/filepath/path_test....
> src/pkg/path/filepath/path_test.go:442: {"abc/def", "abc"},
> Hmm.. I do wonder about this case, on a second look. Should this have a
> trailing slash, to match Split's behavior? It may be slightly surprising
> that the result of Split(path) and Dir(path) yield different dir values.

this was how the whole thing started, with someone complaining about the
trailing slash. i can't make everyone happy, it seems. at least the behavior is
documented.

what do you want? i'll do what you want. it's just a +1 on the string to put it
back, but it's much clunkier to remove.

-rob

[rsc, 2011-12-22 19:20:29]

I think Dir should return the same form that Clean does,
which means no trailing slash except for the root directory.
That is not always what you want, but neither is the other.

Russ

[r2, 2011-12-22 19:21:43]

On Dec 22, 2011, at 11:20 AM, Russ Cox wrote:

> I think Dir should return the same form that Clean does,
> which means no trailing slash except for the root directory.
> That is not always what you want, but neither is the other.

That's the documented behavior of the implementation here.

-rob

[rsc, 2011-12-22 19:25:50]

Code looks good.

The comment is not quite right: Dir(`/foo`) = `/` but it
doesn't fall under the specified case.

Also, on windows presumably Dir(`c:\foo`) = `c:\`.

Maybe

The returned path does not end in a separator unless it
is the root directory.

Russ

[rsc, 2011-12-22 19:26:03]

Please add a test for /foo too.

[gustavo_niemeyer.net, 2011-12-22 19:26:22]

> this was how the whole thing started, with someone complaining about the
trailing slash. i can't make everyone happy, it seems. at least the behavior is
documented.
>
> what do you want? i'll do what you want. it's just a +1 on the string to put
it back, but it's much clunkier to remove.

My vote is to either:

1) Remove the slash from both Split and Dir (behavior seems
preferable, but incompatible if people are blindly assuming the slash)
2) Keep the slash on both

People will probably use Split and Dir interchangeably, and having
them with differing outcomes seems error prone.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[r, 2011-12-22 19:37:19]

Hello golang-dev@googlegroups.com, n13m3y3r@gmail.com, r@google.com,
rsc@golang.org, gustavo@niemeyer.net (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-12-22 19:38:25]

I would prefer not to touch Split.
It has the very nice property that
   dir, file := path.Split(name)
   dir+file == name!

I don't know whether we should keep
Base (and now Dir) different from Split
or make them wrappers around Split
that select one or the other result.

Russ

[r2, 2011-12-22 19:38:40]

On Dec 22, 2011, at 11:25 AM, Gustavo Niemeyer wrote:

>> this was how the whole thing started, with someone complaining about the
trailing slash. i can't make everyone happy, it seems. at least the behavior is
documented.
>> 
>> what do you want? i'll do what you want. it's just a +1 on the string to put
it back, but it's much clunkier to remove.
> 
> My vote is to either:
> 
> 1) Remove the slash from both Split and Dir (behavior seems
> preferable, but incompatible if people are blindly assuming the slash)
> 2) Keep the slash on both
> 
> People will probably use Split and Dir interchangeably, and having
> them with differing outcomes seems error prone.

If Split is going to change, that should happen in a separate CL. I might try
that as an experiment and see if the world salutes.

-rob

[r2, 2011-12-22 19:40:36]

On Dec 22, 2011, at 11:38 AM, Russ Cox wrote:

> I would prefer not to touch Split.
> It has the very nice property that
>   dir, file := path.Split(name)
>   dir+file == name!
> 
> I don't know whether we should keep
> Base (and now Dir) different from Split
> or make them wrappers around Split
> that select one or the other result.

I am happy to leave split as it is and say why.
I am also happy to have Dir drop the trailing slash, since that's almost always
what you want, and it's much easier code-wise to add it back than to take it
off.

-rob

[r, 2011-12-22 19:41:14]

Hello golang-dev@googlegroups.com, n13m3y3r@gmail.com, r@google.com,
rsc@golang.org, gustavo@niemeyer.net (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-12-22 19:51:51]

LGTM
barring any good arguments that might arise

[gustavo_niemeyer.net, 2011-12-22 20:05:59]

> I would prefer not to touch Split.
> It has the very nice property that
>   dir, file := path.Split(name)
>   dir+file == name!

It's an interesting property, but it doesn't seem very useful in
practice, and probably not worth the distinction from Dir.

Note some curious differences, if I understand Dir (haven't run it):

- Case 1:

    dir1, _ := filepath.Split("/a/b/")
    dir2, _ := filepath.Split(dir1)
    println(dir2) // "/a/b/"

    dir1 := filepath.Dir("/a/b/")
    dir2 := filepath.Dir(dir1)
    println(dir1) // "/a

- Case 2 (this looks like a bug in Split):

    dir1, _ := filepath.Split(".")
    println(dir1) // ""

    dir1 := filepath.Dir(".")
    println(dir1) // "."


- Case 3 (this looks like a bug in Dir):

    dir1, _ := filepath.Split("a")
    println(dir1) // ""

    dir1 := filepath.Dir("a")
    println(dir1) // "a"

I really wouldn't like to have to remember what I'm really doing every
time I touch either of them.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[rsc, 2011-12-22 20:10:57]

On Thu, Dec 22, 2011 at 15:05, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> It's an interesting property, but it doesn't seem very useful in
> practice, and probably not worth the distinction from Dir.

Not changing split.  It's the most useful of the bunch.

[r, 2011-12-22 20:58:37]

Hello n13m3y3r@gmail.com, r@google.com, rsc@golang.org, gustavo@niemeyer.net
(cc: golang-dev@googlegroups.com),

Please take another look.

[r2, 2011-12-22 20:59:24]

New attempt, different approach, better results, same idea.

-rob

[rsc, 2011-12-22 21:37:48]

LGTM

Please copy same into path.
I am certain this is wrong for Windows
but not sure how to test `c:\foo` -> `c:\` only
on Windows.

[r2, 2011-12-22 21:43:44]

On Dec 22, 2011, at 1:37 PM, rsc@golang.org wrote:

> LGTM
> 
> Please copy same into path.
> I am certain this is wrong for Windows
> but not sure how to test `c:\foo` -> `c:\` only
> on Windows.

if this is where we're going, i'll do path in a separate CL. is this where we're
going?

-rob

[rsc, 2011-12-22 21:50:23]

On Thu, Dec 22, 2011 at 16:43, Rob 'Commander' Pike <r@google.com> wrote:
> if this is where we're going, i'll do path in a separate CL. is this where
we're going?

i think it's fine.

i think that you can recognize a root if the
last separator is also the first separator
and IsAbs returns true.  that definition
handles / and c:\.

russ

[r, 2011-12-22 21:59:29]

*** Submitted as http://code.google.com/p/go/source/detail?r=f13664d9df19 ***

path/filepath: Dir

There was Base but not Dir, so fill in the gap.

R=n13m3y3r, r, rsc, gustavo
CC=golang-dev
http://codereview.appspot.com/5503067