code review 3458041: io/ioutil: configurable sort mode in ReadDir (issue3458041)

179 views
Skip to first unread message

al...@pbrane.org

unread,
Dec 3, 2010, 8:36:24 PM12/3/10
to r...@golang.org, r...@golang.org, golan...@googlegroups.com, re...@codereview.appspotmail.com
Reviewers: rsc, r,

Message:
Hello rsc, r (cc: golan...@googlegroups.com),

I'd like you to review this change.


Description:
io/ioutil: configurable sort mode in ReadDir

Please review this at http://codereview.appspot.com/3458041/

Affected files:
M src/cmd/godoc/codewalk.go
M src/cmd/godoc/dirtrees.go
M src/cmd/godoc/godoc.go
M src/cmd/godoc/index.go
M src/pkg/io/ioutil/ioutil.go
M src/pkg/io/ioutil/ioutil_test.go
M src/pkg/path/path.go


r...@golang.org

unread,
Dec 3, 2010, 9:52:14 PM12/3/10
to al...@pbrane.org, g...@golang.org, golan...@googlegroups.com, re...@codereview.appspotmail.com

r...@google.com

unread,
Dec 6, 2010, 2:37:21 PM12/6/10
to al...@pbrane.org, g...@golang.org, golan...@googlegroups.com, re...@codereview.appspotmail.com
I am not sure this change is worthwhile.
Sorting by name is almost always what is wanted,
and when not, one can already call the general
sort appropriately (using os.Readdir if necessary
to avoid the cost of the first sort).

ioutil.ReadDir is a convenience wrapper;
as more arguments get added, especially
ones that almost never change, they lose
their convenience and their rationale.

http://codereview.appspot.com/3458041/

Robert Griesemer

unread,
Dec 6, 2010, 7:07:38 PM12/6/10
to al...@pbrane.org, r...@golang.org, r...@golang.org, golan...@googlegroups.com, re...@codereview.appspotmail.com
I agree with Russ; though I can see that on occasion one may want to sort according to time stamps, and perhaps other things. It's informative though that all uses in the golang.org source base only sort by name

I think this CL could be more interesting by changing the API a bit: Instead of providing a sort mode, provide a sort function

func(x, y *os.FileInfo) bool

to ReadDir. If nil is provided, no sorting is happening. There should probably be also a default comparison function for sort by name since it is the common case.

- gri
Reply all
Reply to author
Forward
0 new messages