Grokbase › Groups › Go › golang-dev › December 2012
FAQ
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/

Affected files:
M src/pkg/sort/example_reverse_test.go
A src/pkg/sort/reverse.go
A src/pkg/sort/reverse_test.go

Search Discussions

  • Brad Fitzpatrick at Dec 11, 2012 at 4:57 pm ⇧
    Last time this came up: https://codereview.appspot.com/5496059/
    On Tue, Dec 11, 2012 at 10:04 AM, 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/6909059/>

    Affected files:
    M src/pkg/sort/example_reverse_**test.go
    A src/pkg/sort/reverse.go
    A src/pkg/sort/reverse_test.go

  • Russ Cox at Dec 11, 2012 at 5:00 pm ⇧
    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.
  • Brad Fitzpatrick at Dec 11, 2012 at 5:14 pm ⇧
    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 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.
  • Gri at Dec 11, 2012 at 5:27 pm ⇧
    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_test.go
    File src/pkg/sort/example_reverse_test.go (right):

    https://codereview.appspot.com/6909059/diff/2001/src/pkg/sort/example_reverse_test.go#newcode1
    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#newcode8
    src/pkg/sort/reverse.go:8: // that value. Basic use pattern for reverse
    sorting a int slice:
    s/a/an/ ?

    https://codereview.appspot.com/6909059/
  • Russ Cox at Dec 11, 2012 at 5:29 pm ⇧
    Please use the func Reverse from the issue instead of type Reverse. It
    is fewer moving parts.
  • Gri at Dec 11, 2012 at 5:36 pm ⇧
    rsc's suggestion is the better approach. Please address this as he
    suggested.
    Thanks.
    - gri

    https://codereview.appspot.com/6909059/
  • Remigius Gieben at Dec 11, 2012 at 5:49 pm ⇧

    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/
  • Robert Griesemer at Dec 11, 2012 at 5:44 pm ⇧
    Issue 4511 says it all.
    - gri
    On Tue, Dec 11, 2012 at 9:42 AM, 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/
  • Russ Cox at Dec 11, 2012 at 5:54 pm ⇧
    Note that (in golang.org/issue/4511) func Reverse returns an
    Interface. It does not sort.
  • Remigius Gieben at Dec 11, 2012 at 6:08 pm ⇧

    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

    https://codereview.appspot.com/6909059/
  • Russ Cox at Dec 11, 2012 at 6:25 pm ⇧
    'hg mail' keeps complaining: no files changed.
    FWIW, hg mail CL-number.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 11, '12 at 4:55p
activeDec 11, '12 at 6:25p
posts12
users4
websitegolang.org

4 users in discussion

Russ Cox: 4 posts Robert Griesemer: 3 posts Remigius Gieben: 3 posts Brad Fitzpatrick: 2 posts

Content

People

Support

Translate

site design / logo © 2022 Grokbase