Issue 6909059: sort: add Reverse
|
Created:
11 years, 4 months ago by Miek Gieben Modified:
10 years, 10 months ago CC:
golang-dev Visibility:
Public. |
Descriptionsort: add Reverse
This CL adds a Reverse sort to the package sort.
The new names are quite verbose...
Fixes issue 4511
Patch Set 1 : diff -r 21d62bd46e78 https://code.google.com/p/go/ #
Total comments: 2
Patch Set 2 : code review 6909059: sort: add Reverse #Patch Set 3 : code review 6909059: sort: add Reverse #MessagesTotal messages: 19
Hello, I took a stab at adding a Reverse to the sort package.
Sign in to reply to this message.
Last time this came up: https://codereview.appspot.com/5496059/ On Tue, Dec 11, 2012 at 10:04 AM, <remigius.gieben@gmail.com> wrote: > Reviewers: golang-dev_googlegroups.com, > > Message: > Hello, > > I took a stab at adding a Reverse to the sort package. > > Description: > sort: add Reverse > > This CL adds a Reverse sort to the package sort. > The new names are quite verbose... > Fixes issue 4511 > > Please review this at https://codereview.appspot.**com/6909059/<https://codereview.appspot.com/6909... > > Affected files: > M src/pkg/sort/example_reverse_**test.go > A src/pkg/sort/reverse.go > A src/pkg/sort/reverse_test.go > > >
Sign in to reply to this message.
I objected last time but I filed the bug this time, because I've seen it come up and it's a good example. I don't believe the wrappers should be here, just func Reverse. Happy to bow to popular opinion if you and gri don't believe it should be there.
Sign in to reply to this message.
I wanted Reverse then too, so I don't object. I like the func style more, too. I do object to Float64sAreReverseSorted and friends, though. On Tue, Dec 11, 2012 at 12:00 PM, Russ Cox <rsc@golang.org> wrote: > I objected last time but I filed the bug this time, because I've seen > it come up and it's a good example. I don't believe the wrappers > should be here, just func Reverse. Happy to bow to popular opinion if > you and gri don't believe it should be there. >
Sign in to reply to this message.
LGTM I'm still not a big fan of this - I would probably just add the Reverse type and be done with it. On the other hand it's nice to have the complimentary set of convenience functions. https://codereview.appspot.com/6909059/diff/2001/src/pkg/sort/example_reverse... File src/pkg/sort/example_reverse_test.go (right): https://codereview.appspot.com/6909059/diff/2001/src/pkg/sort/example_reverse... src/pkg/sort/example_reverse_test.go:1: // Copyright 2012 The Go Authors. All rights reserved. usually we don't update the copyrights https://codereview.appspot.com/6909059/diff/2001/src/pkg/sort/reverse.go File src/pkg/sort/reverse.go (right): https://codereview.appspot.com/6909059/diff/2001/src/pkg/sort/reverse.go#newc... src/pkg/sort/reverse.go:8: // that value. Basic use pattern for reverse sorting a int slice: s/a/an/ ?
Sign in to reply to this message.
Please use the func Reverse from the issue instead of type Reverse. It is fewer moving parts.
Sign in to reply to this message.
rsc's suggestion is the better approach. Please address this as he suggested. Thanks. - gri
Sign in to reply to this message.
On 2012/12/11 17:36:52, gri wrote: > rsc's suggestion is the better approach. Please address this as he suggested. > Thanks. > - gri That would mean: don't export the Reverse type, but only the Reverse function?
Sign in to reply to this message.
Issue 4511 says it all. - gri On Tue, Dec 11, 2012 at 9:42 AM, <remigius.gieben@gmail.com> wrote: > On 2012/12/11 17:36:52, gri wrote: >> >> rsc's suggestion is the better approach. Please address this as he > > suggested. >> >> Thanks. >> - gri > > > That would mean: don't export the Reverse type, but only the Reverse > function? > > https://codereview.appspot.com/6909059/
Sign in to reply to this message.
Note that (in golang.org/issue/4511) func Reverse returns an Interface. It does not sort.
Sign in to reply to this message.
On 2012/12/11 17:46:24, rsc wrote: > Note that (in golang.org/issue/4511) func Reverse returns an > Interface. It does not sort. 'hg mail' keeps complaining: no files changed. So I created a new CL: https://codereview.appspot.com/6932054
Sign in to reply to this message.
> 'hg mail' keeps complaining: no files changed. FWIW, hg mail CL-number.
Sign in to reply to this message.
On 2012/12/11 18:24:31, rsc wrote: > > 'hg mail' keeps complaining: no files changed. > > FWIW, hg mail CL-number. Hi Miek, cd $GOROOT/src/pkg/sort; hg mail 6909059 will work.
Sign in to reply to this message.
Ah, in the directory. Completely non-obvious for a heavy git user... On Dec 15, 2012 6:09 AM, <dave@cheney.net> wrote: > On 2012/12/11 18:24:31, rsc wrote: > >> > 'hg mail' keeps complaining: no files changed. >> > > FWIW, hg mail CL-number. >> > > Hi Miek, > > cd $GOROOT/src/pkg/sort; hg mail 6909059 > > will work. > > https://codereview.appspot.**com/6909059/<https://codereview.appspot.com/6909... >
Sign in to reply to this message.
It doesn't have to be in the directory, but it does have to be within $GOROOT, to pick up the code review plugin *. * this isn't always true, but within the context of the instructions on the golang website, it is. On 15 Dec 2012 18:31, "Miek Gieben" <remigius.gieben@gmail.com> wrote: > Ah, in the directory. Completely non-obvious for a heavy git user... > On Dec 15, 2012 6:09 AM, <dave@cheney.net> wrote: > >> On 2012/12/11 18:24:31, rsc wrote: >> >>> > 'hg mail' keeps complaining: no files changed. >>> >> >> FWIW, hg mail CL-number. >>> >> >> Hi Miek, >> >> cd $GOROOT/src/pkg/sort; hg mail 6909059 >> >> will work. >> >> https://codereview.appspot.**com/6909059/<https://codereview.appspot.com/6909... >> >
Sign in to reply to this message.
How are you going Miek ?
Sign in to reply to this message.
I completely failed at getting hg to re-mail the files, so the (much smaller) diff is in https://codereview.appspot.com/6932054 On Fri, Dec 21, 2012 at 5:45 AM, <dave@cheney.net> wrote: > How are you going Miek ? > > https://codereview.appspot.**com/6909059/<https://codereview.appspot.com/6909... >
Sign in to reply to this message.
On 2012/12/21 14:07:16, Miek Gieben wrote: > I completely failed at getting hg to re-mail the files, so the (much > smaller) diff is in https://codereview.appspot.com/6932054 you can do "hg change 6909059" to verify the file list is correct, and then "hg upload 6909059", and see if the patch set is updated correctly. If it's so, "hg mail 6909059" to ask for review. Given that I've already commented that other CL (6932054), please close this CL.
Sign in to reply to this message.
R=closed Submitted in https://code.google.com/p/go/source/detail?r=265bf0259ac6
Sign in to reply to this message.
|
