code review 7067049: path/filepath: add JoinList

path/filepath: add JoinList

For discussion, see https://groups.google.com/d/topic/golang-nuts/PXCr10DsRb4/discussion

[adg, 2013-01-09 06:41:09]

https://codereview.appspot.com/7067049/diff/5001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

https://codereview.appspot.com/7067049/diff/5001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:188: func PathList(elem ...string) string {
filepath.PathList? ugh

https://codereview.appspot.com/7067049/diff/5001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:189: var result []byte
why not just
return strings.Join(path, string(ListSeparator))
?!

[dfc, 2013-01-09 09:27:48]

Thank you for your comments.

https://codereview.appspot.com/7067049/diff/5001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

https://codereview.appspot.com/7067049/diff/5001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:188: func PathList(elem ...string) string {
On 2013/01/09 06:41:09, adg wrote:
> filepath.PathList? ugh

PathList was a previous suggestion, I have reverted to JoinList.

https://codereview.appspot.com/7067049/diff/5001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:189: var result []byte
On 2013/01/09 06:41:09, adg wrote:
> why not just
> return strings.Join(path, string(ListSeparator))
> ?!

While unix copes with lists like

JAVA_CLASSPATH=/a:/b::

I'm not sure that other operating systems with other separators do.

Have a look at the test cases in path_test.go. If you think they are overkill,
then I can make this function simpler.

[dfc, 2013-01-09 09:33:22]

Hello adg@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[adg, 2013-01-09 23:41:28]

Mail this to golang-dev.

[dfc, 2013-01-10 00:29:31]

Hello adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2013-01-10 00:47:28]

Looks fine to me. I'd like to get API sign-off from rsc or iant before
submitting though.

https://codereview.appspot.com/7067049/diff/4006/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

https://codereview.appspot.com/7067049/diff/4006/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:187: // ListSeperator  are split and recombined
internally. Empty elements are
s/  / /

https://codereview.appspot.com/7067049/diff/4006/src/pkg/path/filepath/path_t...
File src/pkg/path/filepath/path_test.go (right):

https://codereview.appspot.com/7067049/diff/4006/src/pkg/path/filepath/path_t...
src/pkg/path/filepath/path_test.go:168: var joinlisttests = []struct {
I see no test for the :: case you called out in the discussion

[adg, 2013-01-10 02:00:19]

https://codereview.appspot.com/7067049/diff/4006/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

https://codereview.appspot.com/7067049/diff/4006/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:193: if l == "" {
empty elements are valid path entries, it seems, so please change the behavior.

also i think now that this is the case, you might as well replace this function
with

if len(elem) == 0 {
  return ""
}
return strings.Join(elem, string(ListSeparator))

[dfc, 2013-01-10 07:48:40]

Please take another look. 

If JoinList were as simple as strings.Join(..., ListSeparator) that would be
great, but the amount of code needed to pass all the test cases, including round
tripping, is worrying. There is one case that I could not solve, "a", ":", "b"
=> "a::b", which is incorrect, because it adds "." to the path.

https://codereview.appspot.com/7067049/diff/4006/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

https://codereview.appspot.com/7067049/diff/4006/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:187: // ListSeperator  are split and recombined
internally. Empty elements are
On 2013/01/10 00:47:28, adg wrote:
> s/  / /

Done.

[kevlar, 2013-01-10 20:47:02]

On 2013/01/10 07:48:40, dfc wrote:
> Please take another look. 
> 
> If JoinList were as simple as strings.Join(..., ListSeparator) that would be
> great, but the amount of code needed to pass all the test cases, including
round
> tripping, is worrying. There is one case that I could not solve, "a", ":", "b"
> => "a::b", which is incorrect, because it adds "." to the path.

If join("a", ":") -> "a:" (which adds . to the path) then why is join("a", ":",
"b") -> "a::b" (or even "a:::b") not correct?  I would expect join(join(x,y),z)
<=> join(x,join(y,z)) <=> join(x,y,z) which is already the case with `return
strings.Join(elem, ListSeparator)`.  The round trips will also be correct if my
mental math is correct.

[dfc, 2013-01-10 21:50:21]

After sleeping on it I think the bette solution is to resolve the ambiguity by
expanding ^: , :$ and :: to .: :. And :.: respectively. This means round trips
are no longer identical, but produce logically comparable results. 

On 11/01/2013, at 7:47, kevlar@google.com wrote:

> On 2013/01/10 07:48:40, dfc wrote:
>> Please take another look.
> 
>> If JoinList were as simple as strings.Join(..., ListSeparator) that
> would be
>> great, but the amount of code needed to pass all the test cases,
> including round
>> tripping, is worrying. There is one case that I could not solve, "a",
> ":", "b"
>> => "a::b", which is incorrect, because it adds "." to the path.
> 
> If join("a", ":") -> "a:" (which adds . to the path) then why is
> join("a", ":", "b") -> "a::b" (or even "a:::b") not correct?  I would
> expect join(join(x,y),z) <=> join(x,join(y,z)) <=> join(x,y,z) which is
> already the case with `return strings.Join(elem, ListSeparator)`.  The
> round trips will also be correct if my mental math is correct.
> 
> https://codereview.appspot.com/7067049/

[dfc, 2013-01-10 22:01:44]

After sleeping on it I think the bette solution is to resolve the ambiguity by
expanding ^: , :$ and :: to .: :. And :.: respectively. This means round trips
are no longer identical, but produce logically comparable results. 

On 11/01/2013, at 7:47, kevlar@google.com wrote:

> On 2013/01/10 07:48:40, dfc wrote:
>> Please take another look.
> 
>> If JoinList were as simple as strings.Join(..., ListSeparator) that
> would be
>> great, but the amount of code needed to pass all the test cases,
> including round
>> tripping, is worrying. There is one case that I could not solve, "a",
> ":", "b"
>> => "a::b", which is incorrect, because it adds "." to the path.
> 
> If join("a", ":") -> "a:" (which adds . to the path) then why is
> join("a", ":", "b") -> "a::b" (or even "a:::b") not correct?  I would
> expect join(join(x,y),z) <=> join(x,join(y,z)) <=> join(x,y,z) which is
> already the case with `return strings.Join(elem, ListSeparator)`.  The
> round trips will also be correct if my mental math is correct.
> 
> https://codereview.appspot.com/7067049/

[adg, 2013-01-10 22:58:14]

On 10 January 2013 18:48, <dave@cheney.net> wrote:

> There is one case that I
> could not solve, "a", ":", "b" => "a::b", which is incorrect, because it
> adds "." to the path.
>

JoinList([]string{"a", ":", "b"}) == "a:::b"
and that seems fine to me.

Why would someone put ":" in there, anyway?

[adg, 2013-01-10 22:58:43]

On 11 January 2013 08:49, Dave Cheney <dave@cheney.net> wrote:

> I think the bette solution is to resolve the ambiguity by expanding ^: ,
> :$ and :: to .: :. And :.: respectively.


I think we shouldn't mess with the data put in there by the user. If they
want to add :: to a path, so be it. We shouldn't assume they meant dot.

[dfc, 2013-01-10 23:08:21]

On Fri, Jan 11, 2013 at 9:57 AM, Andrew Gerrand <adg@golang.org> wrote:
>
> On 10 January 2013 18:48, <dave@cheney.net> wrote:
>>
>> There is one case that I
>> could not solve, "a", ":", "b" => "a::b", which is incorrect, because it
>> adds "." to the path.
>
>
> JoinList([]string{"a", ":", "b"}) == "a:::b"
> and that seems fine to me.
>
> Why would someone put ":" in there, anyway?

By mistake, this is something that filepath.Join guards against,
http://play.golang.org/p/kNZXYQW8wH, so I think JoinList has to guard
against it.

PeterS / speter is right, JoinPath mustn't insert a cwd entry into the
path while joining unless it was there already. If it does, then it is
no better than strings.Join(..., ListSeparator), and should not be in
the std library.

I'm going to take one more swing at this and if that dones't work out,
abandon the idea. While I dont' like the idea of people hand rolling
their own solution for this, we discourage string munging where
filepath.Join() is appropriate, if I can't come up with a correct
JoinList implementation, it doesn't belong in the std lib.

Cheers

Dave

[kevlar, 2013-01-11 18:51:30]

On Thu, Jan 10, 2013 at 6:08 PM, Dave Cheney <dave@cheney.net> wrote:

> On Fri, Jan 11, 2013 at 9:57 AM, Andrew Gerrand <adg@golang.org> wrote:
> >
> > On 10 January 2013 18:48, <dave@cheney.net> wrote:
> >>
> >> There is one case that I
> >> could not solve, "a", ":", "b" => "a::b", which is incorrect, because it
> >> adds "." to the path.
> >
> >
> > JoinList([]string{"a", ":", "b"}) == "a:::b"
> > and that seems fine to me.
> >
> > Why would someone put ":" in there, anyway?
>
> By mistake, this is something that filepath.Join guards against,
> http://play.golang.org/p/kNZXYQW8wH, so I think JoinList has to guard
> against it.
>

I think that's because a///b is a/b when you clean the path; a:::b is not
equivalent to a:b.


> PeterS / speter is right, JoinPath mustn't insert a cwd entry into the
> path while joining unless it was there already. If it does, then it is
> no better than strings.Join(..., ListSeparator), and should not be in
> the std library.
>
> I'm going to take one more swing at this and if that dones't work out,
> abandon the idea. While I dont' like the idea of people hand rolling
> their own solution for this, we discourage string munging where
> filepath.Join() is appropriate, if I can't come up with a correct
> JoinList implementation, it doesn't belong in the std lib.
>
> Cheers
>
> Dave
>

[adg, 2013-01-14 00:29:20]

On 11 January 2013 10:08, Dave Cheney <dave@cheney.net> wrote:

> PeterS / speter is right, JoinPath mustn't insert a cwd entry into the
> path while joining unless it was there already. If it does, then it is
> no better than strings.Join(..., ListSeparator), and should not be in
> the std library.
>

I'm confused by this argument.

  strings.Join([]string{"a:", "b"}, ListSeparator)

does not introduce a cwd entry. It's already there. In "a:", which is a
list of "a" and "" (cwd).

I think we can just say "JoinList joins a list of paths or path lists into
a single path list string, separated by ListSeparator." This means that
list elements containing the list separator should be considered lists
themselves.

Trying to escape the list separator character is not worth our while, and
we have precedent: SplitList doesn't do it.

Andrew

[dfc, 2013-01-14 11:44:35]

Sorry for the delay. I was not able to work on this proposal today. I
hope to have time tomorrow evening.

On Mon, Jan 14, 2013 at 11:28 AM, Andrew Gerrand <adg@golang.org> wrote:
>
> On 11 January 2013 10:08, Dave Cheney <dave@cheney.net> wrote:
>>
>> PeterS / speter is right, JoinPath mustn't insert a cwd entry into the
>> path while joining unless it was there already. If it does, then it is
>> no better than strings.Join(..., ListSeparator), and should not be in
>> the std library.
>
>
> I'm confused by this argument.
>
>   strings.Join([]string{"a:", "b"}, ListSeparator)
>
> does not introduce a cwd entry. It's already there. In "a:", which is a list
> of "a" and "" (cwd).
>
> I think we can just say "JoinList joins a list of paths or path lists into a
> single path list string, separated by ListSeparator." This means that list
> elements containing the list separator should be considered lists
> themselves.
>
> Trying to escape the list separator character is not worth our while, and we
> have precedent: SplitList doesn't do it.
>
> Andrew

[rsc, 2013-01-31 01:41:45]

https://codereview.appspot.com/7067049/diff/7003/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

https://codereview.appspot.com/7067049/diff/7003/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:185: // JoinList joins any number of list elements
into a single list, adding an
On Unix, I think this should do:

func JoinList(elem ...string) string {
    return strings.Join(elem, string(ListSeparator))
}

Treating individual elem[i] as if they were implicit lists to be split and
rejoined causes confusing ambiguity. Worse, on Windows we apparently have to
deal with the fact that lists can have the separator, quoted, in an element. So
SplitList is going to have to unquote and JoinList is going to have to requote.

[adg, 2013-12-03 06:16:25]

I went to use this function today and it's not there. What happened?

[dfc, 2013-12-03 06:21:34]

On 2013/12/03 06:16:25, adg wrote:
> I went to use this function today and it's not there. What happened?

This CL is dead, but there may be some movement here
https://groups.google.com/d/msg/golang-nuts/PXCr10DsRb4/NmzPNTrwXj0J

[gobot, 2013-12-20 16:21:41]

Replacing golang-dev with golang-codereviews.

[dfc, 2014-01-01 07:19:43]

*** Abandoned ***