code review 4964067: path/filepath: Simplify Walk interface

path/filepath: Simplify Walk interface

The last argument of filepath.Walk was removed, and the Visitor
interface now contains an Error method that is called on errors.

Fixes issue 2237.

[niemeyer, 2011-09-07 20:53:20]

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

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

[gri, 2011-09-07 21:05:06]

You beat me to it.

Looks pretty good, but I have a couple of comments.

- gri

http://codereview.appspot.com/4964067/diff/5001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/4964067/diff/5001/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:258: // visited by Walk. The parameter path is the
full path of f relative
I think this might not be right anymore. E.g. if there's an error right in the
beginning (say, Walk("nonexisting_dir", v), then error is not called with path =
".", that is, it is not relative to root.

http://codereview.appspot.com/4964067/diff/5001/src/pkg/path/filepath/path_te...
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/4964067/diff/5001/src/pkg/path/filepath/path_te...
src/pkg/path/filepath/path_test.go:302: type TestVisitor struct {
should this just be a chan os.Error now?

[niemeyer, 2011-09-07 21:31:33]

> You beat me to it.

Sorry for jumping ahead. Want to use the new interface. :)

> I think this might not be right anymore.

Good catch. It looks like this was actually never true for 
any of the functions, since there's no logic to make the
path relative. Fixed the doc.

> should this just be a chan os.Error now?

Done.

PTAL

[gri, 2011-09-07 21:45:18]

LGTM

Thanks for doing this.
- gri

PS: It would be nice to have a gofix module for this, but I fear w/o more type
info this is not easily possible (except for removing the chan parameter to
Walk, but it still requires manual fixes).

[r2, 2011-09-07 21:46:28]

please hold off

i'm not happy with this.

-rob

[gri, 2011-09-07 21:49:54]

*** Submitted as http://code.google.com/p/go/source/detail?r=661cb84cc6f0 ***

path/filepath: Simplify Walk interface

The last argument of filepath.Walk was removed, and the Visitor
interface now contains an Error method that is called on errors.

Fixes issue 2237.

R=golang-dev, gri, r
CC=golang-dev
http://codereview.appspot.com/4964067

Committer: Robert Griesemer <gri@golang.org>

[rsc, 2011-09-07 21:51:22]

Hang on please.  Let's see if others have different, maybe better ideas.

[rsc, 2011-09-07 21:52:19]

Can we please hg undo this?
This is an API change and I don't think we
want it in this weekly without knowing it's
the change we want to make.

[r, 2011-09-07 21:59:24]

On Wed, Sep 7, 2011 at 2:52 PM, Russ Cox <rsc@golang.org> wrote:
> Can we please hg undo this?
> This is an API change and I don't think we
> want it in this weekly without knowing it's
> the change we want to make.
>

Hey, I said I would look into this. I want to reconsider the API
because I think something much better overall should exist.  Please
revert this change.

-rob

[gustavo_niemeyer.net, 2011-09-07 22:07:13]

> Hey, I said I would look into this. I want to reconsider the API
> because I think something much better overall should exist.  Please
> revert this change.

Sorry about that.  Wasn't trying to rule over your request.. I thought
there was already an internal agreement it was fine to go ahead with
this given further comments on the issue.

Robert, Russ and I seem to agree this is a simple yet useful API
change, but I'm keen on learning your idea as well if you see
something entirely different with the same benefits.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[r, 2011-09-07 22:10:30]

I don't see quite the anonymity, but more to the point this was an API
change that happened pretty fast, after I said I'd look into a
solution.  Moreover, this is an API change to fix a bug in the
implementation, which is a clue the issue requires more thought.

By the way, API change CLs should come with gofix modules. Ironically,
you changed gofix/main.go but didn't add a module.

Again, please pull this out for now.

-rob

[r, 2011-09-07 22:12:34]

ha ha
s/anonymity/unanimity/

-rob

[gustavo_niemeyer.net, 2011-09-07 22:15:56]

> I don't see quite the unanimity, but more to the point this was an API

Please read the thread in the issue.

> change that happened pretty fast, after I said I'd look into a
> solution.  Moreover, this is an API change to fix a bug in the
> implementation, which is a clue the issue requires more thought.
>
> By the way, API change CLs should come with gofix modules. Ironically,
> you changed gofix/main.go but didn't add a module.

Because indeed I wasn't planning on adding one due to the nature of
the change. I accept critiques on the choice, but it wasn't an
overlook.

> Again, please pull this out for now.

Doing that now.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[r, 2011-09-07 22:19:34]

LGTM
thanks

[gri, 2011-09-07 22:28:27]

This is entirely my fault - I should have solicited more feedback. My
apologies.

That said, the changes required were so straight-forward (and existing code
so easy to adjust) that I am inclined to think this is not a bad change and
even a natural API for the particular problem. But there may be a more
general over-arching principle that applies elsewhere.

I have thought about the gofix issue (and mentioned it in the code review),
but except for the parameter passing change to Walk, I'm not sure that the
gofix is strong enough to do the change in general. Only removing the
parameter is not going to make a difference - the compiler will still
complain about the missing Error method.

- gri

On Wed, Sep 7, 2011 at 2:59 PM, Rob 'Commander' Pike <r@golang.org> wrote:

> On Wed, Sep 7, 2011 at 2:52 PM, Russ Cox <rsc@golang.org> wrote:
> > Can we please hg undo this?
> > This is an API change and I don't think we
> > want it in this weekly without knowing it's
> > the change we want to make.
> >
>
> Hey, I said I would look into this. I want to reconsider the API
> because I think something much better overall should exist.  Please
> revert this change.
>
> -rob
>

[rsc, 2011-09-08 02:03:36]

On Wed, Sep 7, 2011 at 18:28, Robert Griesemer <gri@golang.org> wrote:
> I have thought about the gofix issue (and mentioned it in the code review),
> but except for the parameter passing change to Walk, I'm not sure that the
> gofix is strong enough to do the change in general. Only removing the
> parameter is not going to make a difference - the compiler will still
> complain about the missing Error method.

Even with a working gofix, I think it would still help to
have more time to think about which API change to make.

Russ