rogpeppe / go-internal

Selected Go-internal packages factored out from the standard library
BSD 3-Clause "New" or "Revised" License
823 stars 67 forks source link

Add golang.org/x/tools/internal/fastwalk #232

Open dolmen opened 1 year ago

dolmen commented 1 year ago

Copy golang.org/x/tools/internal/fastwalk as github.com/rogpeppe/go-internal/fastwalk.

mvdan commented 1 year ago

Could you please expand a bit on why you'd want it? At least personally, when I want something faster than filepath.Walk (i.e. with fewer syscalls), I use https://pkg.go.dev/path/filepath#WalkDir. I wonder why that package makes no mention of it.

mvdan commented 1 year ago

In fact, fastwalk appears to have zero importers, so perhaps it's entirely unused and should be removed.

dolmen commented 1 year ago
  1. I found that fastwalk had been copied in module github.com/mattn/go-zglob a long time ago and that it would be good to upgrade it. But I thought that maintaining/synchronizing that copy would be better done in go-internal, especially if this repo provides tooling to ease such synchronization work. Once fastwalk is available here, I intend to submit a PR to go-zglob that will use it.
  2. golang.org/x/tools/internal/fastwalk is imported by golang.org/x/tools/internal/gopathwalk which is imported by golang.org/x/tools/internal/imports which is imported by golang.org/x/tools/imports and golang.org/x/tools/gopls
  3. I'm ready to do the import job myself and submit a PR.
mvdan commented 1 year ago

1 - why is go-zglob not simply using https://pkg.go.dev/path/filepath#WalkDir?

2 - you're right that fastwalk is used by x/tools; pkg.go.dev is wrong about "zero importers". I asked them about it, and they said they wrote this package before filepath.WalkDir existed, and so it's probably obsolete by now. It's just not high priority to do that, I assume, given that it's an internal package.

3 - the work is not the problem :) I mainly do not want to add a package if the need is already met by the standard library. In this case, I'm pretty sure it already is.

bep commented 8 months ago

@mvdan I have no strong meaning as to whether fastwalk is useful, though I just woould chime in that fastfalk also

I suspect at least the first bullet is important for gopls.