code review 198049: Changed path.Join to allow any number or args.

Changed path.Join to allow any number or args.

I changed path.Join so it would accept any number of
directories as arguments. I used the new ... T syntax. It is
also completely backwards compatable with the old function.

[smwca, 2010-02-02 01:40:34]

Hello rsc, r (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[r, 2010-02-02 01:49:27]

http://codereview.appspot.com/198049/diff/4/5
File src/pkg/path/path.go (right):

http://codereview.appspot.com/198049/diff/4/5#newcode120
src/pkg/path/path.go:120: func Join(dir ...string) string {
nice signature.

http://codereview.appspot.com/198049/diff/4/5#newcode123
src/pkg/path/path.go:123: for i := 0; i < len(dir); i++ {
why bother?  Clean will take care of the extra slashes.

[rsc, 2010-02-02 01:57:16]

there should be tests.

[smwca, 2010-02-02 03:04:37]

@rsc
I will make tests tomorrow. I have never done it before but I can learn.

@r
I ignore empty strings in the beginning of the slice because I do not want it to
make absolute paths accedentally. I do not think it would be good if
path.Join("", "1", "2") returned "/1/2". Clean removes the rest of the empty
strings. So path.Join("1", "", "2") returns "1/2".

-- Stephen

[rsc, 2010-02-02 06:46:19]

On Mon, Feb 1, 2010 at 19:04, <stephen@q5comm.com> wrote:

> @rsc
> I will make tests tomorrow. I have never done it before but I can learn.
>

Thanks.  One important test is what happens when all the arguments
are empty.  It looks to me like the current code will return "/".

Russ

[smwca, 2010-02-03 01:07:59]

> Thanks.  One important test is what happens when all the arguments
> are empty.  It looks to me like the current code will return "/".

Thanks, I did not see that. Also, it does not return a slash, it returns a dot.
Path.Clean() works in misterious ways.

[smwca, 2010-02-03 03:16:41]

Hello rsc, r (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[smwca, 2010-02-03 03:21:04]

I am sorry. I am new to the codereview system. How can I add
src/pkg/path/path_test.go to the list of files?

All I did was add a test for "" and "" as input to the test program and fixed
the problem in the pkg.

[r2, 2010-02-03 03:24:30]

On Feb 3, 2010, at 2:21 PM, stephen@q5comm.com wrote:

> I am sorry. I am new to the codereview system. How can I add
> src/pkg/path/path_test.go to the list of files?
>
> All I did was add a test for "" and "" as input to the test program  
> and
> fixed the problem in the pkg.
>
> http://codereview.appspot.com/198049/show

% hg help file
hg file [-d] change# FILE ...

assign files to or remove files from a change list

	Assign files to or (with -d) remove files from a change list.

	The -d option only removes files from the change list.
	It does not edit them or remove them from the repository.

-rob

[smwca, 2010-02-03 03:26:34]

All my changes are up.

-- Stephen

[rsc, 2010-02-03 03:27:24]

On Tue, Feb 2, 2010 at 19:21, <stephen@q5comm.com> wrote:

> I am sorry. I am new to the codereview system. How can I add
> src/pkg/path/path_test.go to the list of files?
>
> All I did was add a test for "" and "" as input to the test program and
> fixed the problem in the pkg.


No worries.  In addition to the "hg file N filename" command that Rob
pointed out,
you can also run "hg change N" to edit the list of files in a text editor.

Russ

[smwca, 2010-02-03 21:08:38]

bump

[rsc, 2010-02-03 22:05:43]

On Wed, Feb 3, 2010 at 13:08,  <stephen@q5comm.com> wrote:
> bump

we'll get there.  i haven't done any >1-line
external code reviews in a few days
due to generating code reviews instead.
i'm planning to do a review batch later today.

russ

[rsc, 2010-02-04 07:40:54]

http://codereview.appspot.com/198049/diff/14/1009
File src/pkg/path/path.go (right):

http://codereview.appspot.com/198049/diff/14/1009#newcode118
src/pkg/path/path.go:118: // Join joins any number of dirs into a single path,
adding a separating
// Join joins any number of path elements into a single path,
// omitting empty elements and adding separating slashes as needed.
func Join(elem ...string) string

(they're not all directories)

http://codereview.appspot.com/198049/diff/14/1009#newcode121
src/pkg/path/path.go:121: // find first index that is not empty
I like the Clean(Join()) approach; you can simplify it further:

for i, e := range elem {
    if e != "" {
        return Clean(strings.Join(elem[e:], "/"))
    }
}
return ""

At that point you don't need comments anymore.

http://codereview.appspot.com/198049/diff/14/1010
File src/pkg/path/path_test.go (right):

http://codereview.appspot.com/198049/diff/14/1010#newcode95
src/pkg/path/path_test.go:95: dir, file, path string
I'd suggest making this

elem []string
path string

http://codereview.appspot.com/198049/diff/14/1010#newcode99
src/pkg/path/path_test.go:99: JoinTest{"a", "b", "a/b"},
JoinTest{[]string{"a", "b"}, "a/b"},
etc

and then at the bottom of the list you can add JoinTests
for other numbers of arguments, like 0 and 1 and 3.

http://codereview.appspot.com/198049/diff/14/1010#newcode108
src/pkg/path/path_test.go:108: 
Clumsy but effective: define join as below and then
call it instead of Join in the test.  Let's you use the
[]string cases defined above.

func join(elem []string, args ...string) string {
    args = elem 
    return Join(args)
}

[smwca, 2010-02-05 00:11:11]

Done.

I am starting to think that I am slowing you down and causing more work.

[rsc, 2010-02-05 00:20:16]

one tiny nit: fix the Join comment,
which says "any number of dirs" but should say "any number of path elements"

[smwca, 2010-02-05 01:44:40]

On 2010/02/05 00:20:16, rsc wrote:
> one tiny nit: fix the Join comment,
> which says "any number of dirs" but should say "any number of path elements"
> 

Done

[rsc, 2010-02-05 10:34:39]

LGTM

[rsc, 2010-02-05 10:39:35]

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

path: make Join variadic

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

Committer: Russ Cox <rsc@golang.org>