code review 7060051: exp/locale/collate: added functionality for sorting.

exp/locale/collate: added functionality for sorting.
Eliminates the need for the user to fiddle with keys.

[mpvl, 2013-01-07 19:27:08]

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

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

[rsc, 2013-01-18 21:59:31]

LGTM

https://codereview.appspot.com/7060051/diff/2001/src/pkg/exp/locale/collate/s...
File src/pkg/exp/locale/collate/sort.go (right):

https://codereview.appspot.com/7060051/diff/2001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:65: // A type that satisfies Lister can be
sorted by Collator's Sort method.
s/type that satisfies //

https://codereview.appspot.com/7060051/diff/2001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:73: // Sort uses sort.Sort to sort the
strings represented by a using the rules of c.
Perhaps name the variable l or x?
Variables named a look like the English article in doc comments.

https://codereview.appspot.com/7060051/diff/2001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:83: // Strings sorts a using the rules of c.
s/a/l/
or x

[mpvl, 2013-01-23 13:11:01]

https://codereview.appspot.com/7060051/diff/2001/src/pkg/exp/locale/collate/s...
File src/pkg/exp/locale/collate/sort.go (right):

https://codereview.appspot.com/7060051/diff/2001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:65: // A type that satisfies Lister can be
sorted by Collator's Sort method.
On 2013/01/18 21:59:31, rsc wrote:
> s/type that satisfies //

Done.

https://codereview.appspot.com/7060051/diff/2001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:73: // Sort uses sort.Sort to sort the
strings represented by a using the rules of c.
On 2013/01/18 21:59:31, rsc wrote:
> Perhaps name the variable l or x?
> Variables named a look like the English article in doc comments.

Done.

https://codereview.appspot.com/7060051/diff/2001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:83: // Strings sorts a using the rules of c.
On 2013/01/18 21:59:31, rsc wrote:
> s/a/l/
> or x

Done.

[mpvl, 2013-01-23 13:16:26]

*** Submitted as https://code.google.com/p/go/source/detail?r=7f25ce26f38f ***

exp/locale/collate: added functionality for sorting.
Eliminates the need for the user to fiddle with keys.

R=rsc
CC=golang-dev
https://codereview.appspot.com/7060051

[r, 2013-02-21 00:08:29]

https://codereview.appspot.com/7060051/diff/8001/src/pkg/exp/locale/collate/s...
File src/pkg/exp/locale/collate/sort.go (right):

https://codereview.appspot.com/7060051/diff/8001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:38: func (s *sorter) clean() {
where is this used?

https://codereview.appspot.com/7060051/diff/8001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:65: // A Lister can be sorted by Collator's
Sort method.
why is this a Lister? odd name.

https://codereview.appspot.com/7060051/diff/8001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:83: // Strings sorts x using the rules of c.
rephrase to be like the comment for the previous method

https://codereview.appspot.com/7060051/diff/8001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:84: func (c *Collator) Strings(x []string) {
should this be called SortStrings?

https://codereview.appspot.com/7060051/diff/8001/src/pkg/exp/locale/collate/s...
File src/pkg/exp/locale/collate/sort_test.go (right):

https://codereview.appspot.com/7060051/diff/8001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort_test.go:16: "ad",
put "ab" in here for fun.

https://codereview.appspot.com/7060051/diff/8001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort_test.go:41: strings := []string{
strings := sorter {
}

then you don't need the conversion below

by analogy with sort.StringSlice etc., you might want to export this type.

[mpvl, 2013-02-21 17:16:39]

changed added in CL 7402048.

See comments on the use of Lister, though.

https://codereview.appspot.com/7060051/diff/8001/src/pkg/exp/locale/collate/s...
File src/pkg/exp/locale/collate/sort.go (right):

https://codereview.appspot.com/7060051/diff/8001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:38: func (s *sorter) clean() {
good catch.  no longer used.
On 2013/02/21 00:08:29, r wrote:
> where is this used?

https://codereview.appspot.com/7060051/diff/8001/src/pkg/exp/locale/collate/s...
src/pkg/exp/locale/collate/sort.go:65: // A Lister can be sorted by Collator's
Sort method.
On 2013/02/21 00:08:29, r wrote:
> why is this a Lister? odd name.
It is not really a sorter, as it doesn't compare.  It merely provides access to
text in a list and allows for swapping.  Any suggestions for another name?

[r, 2013-02-21 18:57:10]

i can't think of one, so leave it as is but please explain the name in
the comment.