code review 4657050: strings, bytes: Introducing shortuct splitting functions.

strings, bytes: Introducing shortuct splitting functions.

The semantic of the third argument of Split and SplitAfter functions can be confusing in the
case of negative value. I added two shortcut functions SplitAll and SplitAfterAll that clearly
claim their intent. Meanwhile, I fixed a little typo in the SplitAfter documentation.

[vk, 2011-06-25 21:41:33]

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

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

[r, 2011-06-25 22:11:06]

I'm not happy with adding lots of one-line helper functions to the library. They
add clutter and are more helpful for beginners than people who know the
language.

On the other hand, I do have some sympathy for the complaint about the ugliness
of the final argument to Split. I am leaning towards accepting this with the
understanding that it is addressing the ugliness rather than opening the door to
zillions of variant functions.  But I want to think about it some more.

[r, 2011-06-25 22:16:18]

http://codereview.appspot.com/4657050/diff/2001/src/pkg/strings/strings.go
File src/pkg/strings/strings.go (right):

http://codereview.appspot.com/4657050/diff/2001/src/pkg/strings/strings.go#ne...
src/pkg/strings/strings.go:227: // SplitAfterAll slices s into substrings after
each instance of sep and
"split after all" reads funny. i think SplitAllAfter is a better name.

http://codereview.appspot.com/4657050/diff/2001/src/pkg/strings/strings_test.go
File src/pkg/strings/strings_test.go (right):

http://codereview.appspot.com/4657050/diff/2001/src/pkg/strings/strings_test....
src/pkg/strings/strings_test.go:243: if tt.n < 0 {
this is nesting too deep. invert.
if tt.n >= 0 {
   continue
}
or, perhaps even better, move this special case test into the main test.

[r, 2011-06-25 23:23:14]

http://codereview.appspot.com/4657050/diff/2001/src/pkg/strings/strings.go
File src/pkg/strings/strings.go (right):

http://codereview.appspot.com/4657050/diff/2001/src/pkg/strings/strings.go#ne...
src/pkg/strings/strings.go:227: // SplitAfterAll slices s into substrings after
each instance of sep and
On 2011/06/25 22:16:18, r wrote:
> "split after all" reads funny. i think SplitAllAfter is a better name.

changed my mind on this. the name is funny but it's consistent, so leave it.

[jan_mercl, 2011-06-26 08:52:46]

On 2011/06/25 22:11:06, r wrote:
> I'm not happy with adding lots of one-line helper functions to the library.
They
> add clutter and are more helpful for beginners than people who know the
> language.
> 
> On the other hand, I do have some sympathy for the complaint about the
ugliness
> of the final argument to Split. I am leaning towards accepting this with the
> understanding that it is addressing the ugliness rather than opening the door
to
> zillions of variant functions.  But I want to think about it some more.

I would like to suggest: If the change is motivated by the possible confusion of
the beginners, then make the common case, which they are used to from some other
languages, the "all" case. I.e. let Split perform what the -1 'n' parameter does
now. The less common case, where N splits are to be performed, can be then named
e.g. SplitN or SplitMax or ... BTW, this will avoid the bit funny SplitAfterAll,
though I agree with the latest Rob notice about the actual consistency of that
name.

This all will be somewhat in line with e.g. Python's:

str.split([sep[, maxsplit]])

Return a list of the words in the string, using sep as the delimiter string. If
maxsplit is given, at most maxsplit splits are done (thus, the list will have at
most maxsplit+1 elements). If maxsplit is not specified, then there is no limit
on the number of splits (all possible splits are made).
(ref: http://docs.python.org/library/stdtypes.html )

A variation of this suggestion can also be a even more Python-like:

func Split(s, sep string, maxsplit ...int) {
    switch len(maxsplit) {
    case 0:
        // perform unlimited Split
    case 1:
        // perform max maxsplit[0] Split
    default:
        // KABOOM!
    }
}

But this is IMO ugly as well in Go. It abuses the vararg concept to some extent
and can produce more confusion for anyone accidentally/mistakenly writing
Split(s, sep, 1, 2, 3) - with a "Why I get a run time panic here?" flooding of
golang-nuts.

[vk, 2011-06-26 19:09:00]

Hi,

My first intent was to get rid of the IMO ugly -1 parameter that I encountered
in most of my usages of the Split function. I thought about renaming Split in
SplitN and define Split(s, sep string) instead, but that would have broken
existent client code and I was not able to determine if a gofix patch would have
been desirable for such a minor change.

BTW this is my very first open source contribution, so I mechanically applied
the guidelines and thus, I don't know what to do next. May (can) I apply other
changes in this changeset to take Rob's comments into account (especially those
concerning the tests), or shall I submit another change ?

Regards

V.

[cainetighe, 2011-06-26 21:44:43]

yes, you can just make more changes based on feedback and then run "hg mail"
again.

(phone)

On Jun 26, 2011, at 3:09 PM, valentin.kasas@gmail.com wrote:

> Hi,
> 
> My first intent was to get rid of the IMO ugly -1 parameter that I
> encountered in most of my usages of the Split function. I thought about
> renaming Split in SplitN and define Split(s, sep string) instead, but
> that would have broken existent client code and I was not able to
> determine if a gofix patch would have been desirable for such a minor
> change.
> 
> BTW this is my very first open source contribution, so I mechanically
> applied the guidelines and thus, I don't know what to do next. May (can)
> I apply other changes in this changeset to take Rob's comments into
> account (especially those concerning the tests), or shall I submit
> another change ?
> 
> Regards
> 
> V.
> 
> http://codereview.appspot.com/4657050/

[adg, 2011-06-27 01:08:05]

On 26 June 2011 08:16,  <r@golang.org> wrote:
>
> http://codereview.appspot.com/4657050/diff/2001/src/pkg/strings/strings.go
> File src/pkg/strings/strings.go (right):
>
>
http://codereview.appspot.com/4657050/diff/2001/src/pkg/strings/strings.go#ne...
> src/pkg/strings/strings.go:227: // SplitAfterAll slices s into
> substrings after each instance of sep and
> "split after all" reads funny. i think SplitAllAfter is a better name.

What if this new function were Split and the existing Split was
renamed to SplitN (where n is the number of splits to make).

It's a backward incompatible change, but a simple one that's easy to gofix.

I'm happy to leave it as-is (one Split function), though.

Andrew

[dsymonds, 2011-06-27 01:14:59]

This discussion gives me flashbacks.
  http://code.google.com/p/go/source/detail?r=e23ff864e1ae

Wow, almost exactly two years ago!

[r2, 2011-06-27 01:30:11]

On Jun 27, 2011, at 11:07 AM, Andrew Gerrand wrote:

> On 26 June 2011 08:16,  <r@golang.org> wrote:
>> 
>> http://codereview.appspot.com/4657050/diff/2001/src/pkg/strings/strings.go
>> File src/pkg/strings/strings.go (right):
>> 
>>
http://codereview.appspot.com/4657050/diff/2001/src/pkg/strings/strings.go#ne...
>> src/pkg/strings/strings.go:227: // SplitAfterAll slices s into
>> substrings after each instance of sep and
>> "split after all" reads funny. i think SplitAllAfter is a better name.
> 
> What if this new function were Split and the existing Split was
> renamed to SplitN (where n is the number of splits to make).
> 
> It's a backward incompatible change, but a simple one that's easy to gofix.
> 
> I'm happy to leave it as-is (one Split function), though.

it reminds me a bit of the os.Open discussion and i think this is the way to go.

i'll do the work, since it involves a gofix module.

valentin: please abandon this CL (hg change -d 4657050).

-rob

[rog, 2011-06-27 07:04:59]

If this is to happen, I think I'd prefer the current functions to be renamed
SplitN and SplitAfterN, so that we could keep the current names for the more
common case of splitting all.
On 26 Jun 2011 10:52, <befelemepeseveze@gmail.com> wrote:
> On 2011/06/25 22:11:06, r wrote:
>> I'm not happy with adding lots of one-line helper functions to the
> library. They
>> add clutter and are more helpful for beginners than people who know
> the
>> language.
>
>> On the other hand, I do have some sympathy for the complaint about the
> ugliness
>> of the final argument to Split. I am leaning towards accepting this
> with the
>> understanding that it is addressing the ugliness rather than opening
> the door to
>> zillions of variant functions. But I want to think about it some
> more.
>
> I would like to suggest: If the change is motivated by the possible
> confusion of the beginners, then make the common case, which they are
> used to from some other languages, the "all" case. I.e. let Split
> perform what the -1 'n' parameter does now. The less common case, where
> N splits are to be performed, can be then named e.g. SplitN or SplitMax
> or ... BTW, this will avoid the bit funny SplitAfterAll, though I agree
> with the latest Rob notice about the actual consistency of that name.
>
> This all will be somewhat in line with e.g. Python's:
>
> str.split([sep[, maxsplit]])
>
> Return a list of the words in the string, using sep as the delimiter
> string. If maxsplit is given, at most maxsplit splits are done (thus,
> the list will have at most maxsplit+1 elements). If maxsplit is not
> specified, then there is no limit on the number of splits (all possible
> splits are made).
> (ref: http://docs.python.org/library/stdtypes.html )
>
> A variation of this suggestion can also be a even more Python-like:
>
> func Split(s, sep string, maxsplit ...int) {
> switch len(maxsplit) {
> case 0:
> // perform unlimited Split
> case 1:
> // perform max maxsplit[0] Split
> default:
> // KABOOM!
> }
> }
>
> But this is IMO ugly as well in Go. It abuses the vararg concept to some
> extent and can produce more confusion for anyone accidentally/mistakenly
> writing Split(s, sep, 1, 2, 3) - with a "Why I get a run time panic
> here?" flooding of golang-nuts.
>
> http://codereview.appspot.com/4657050/

[adg, 2011-06-27 07:07:34]

On 27 June 2011 17:04, roger peppe <rogpeppe@gmail.com> wrote:
> If this is to happen, I think I'd prefer the current functions to be renamed
> SplitN and SplitAfterN, so that we could keep the current names for the more
> common case of splitting all.

rog: http://codereview.appspot.com/4661051/