Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/tools/gopls: support matching directories at arbitrary depth using the directoryFilters setting #46438

Closed
cespare opened this issue May 29, 2021 · 13 comments
Closed

x/tools/gopls: support matching directories at arbitrary depth using the directoryFilters setting #46438

cespare opened this issue May 29, 2021 · 13 comments
Assignees
@dle8
Labels
FeatureRequest FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.9.2

Comments

@cespare
Copy link
Contributor

cespare commented May 29, 2021

I have some very large node_modules trees that cause gopls to fail. The recently-added directoryFilters configuration setting is a partial workaround for this, but the paths it accepts are always interpreted relative to the workspace root.

There are multiple large node_modules directories in my workspace and the set might change in the future. I don't want to keep directoryFilters in sync with this. Instead, I want to globally ignore node_modules directories everywhere, in all workspaces, forever. But there doesn't seem to be any way to specify an unrooted path in directoryFilters.

I suggest adding a way to do this. One question is the syntax to use. If we were designing this from scratch, perhaps a rooted path (-/myproj/node_modules) could indicate workspace-relative and unrooted path (-node_modules) could indicate "match this directory at any depth". (This would be similar to how, for example, gitignore works.)

But since that option is foreclosed at this point, here are some other ideas to get us started. A leading * could be used to indicate "match at any depth": -*/node_modules. That resembles, yet is confusingly different from, shell glob syntax, so perhaps * could mean "match any single directory" and ** could mean "match any depth"; then I could use -**/node_modules.
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels May 29, 2021
@gopherbot gopherbot added this to the Unreleased milestone May 29, 2021
@myitcv
Copy link
Member

myitcv commented May 29, 2021

In the somewhat related #41504 (comment) I suggested using the .gitignore approach - that would seem to work here too?

@stamblerre
Copy link
Contributor

/cc @heschi here for input, since he designed this feature

@stamblerre stamblerre modified the milestones: Unreleased, gopls/unplanned Jun 1, 2021
@heschi
Copy link
Contributor

heschi commented Jun 1, 2021

As you say, it's moot now, but FWIW I think using / for the project root would be very ambiguous with absolute paths, which is why I started with relative paths. I suspect it's not a problem in the context of a Git repository, but editor settings have a much weaker context.

I'm in favor of using ** to mean match any number of folders, including 0; I just didn't get around to implementing it.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/347297 mentions this issue: internal/lsp: exclude node_modules in the workspace root by default

gopherbot pushed a commit to golang/tools that referenced this issue Sep 2, 2021
@stamblerre
internal/lsp: exclude node_modules in the workspace root by default
3b801c8 
It is unlikely that users want gopls operating on their node_modules
directories, so we should exclude them by default. If a user wants to
include them, they can override their directory filters setting.

This doesn't exclude *any* directory named "node_modules", so we still
need to implement golang/go#46438 to exclude node_modules completely.

Change-Id: I03c42208e62390dc35e44ac5176422ddf8dc53f7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/347297
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@findleyr findleyr mentioned this issue Sep 3, 2021
Open
@cespare cespare mentioned this issue Sep 3, 2021
Open
@stamblerre stamblerre self-assigned this Sep 10, 2021
@stamblerre stamblerre removed their assignment Oct 19, 2021
@adonovan
Copy link
Member

Let's use ... (which must be a complete slash-delimited segment of the pattern) to mean "zero or more complete segments of the file name", just as we do in the go command (e.g. go build ./foo/...)

@hyangah
Copy link
Contributor

hyangah commented Jun 23, 2022

@adonovan the main motivation of this issue is to specify rules like "exclude any directory that has name 'node_modules'" (e.g. node_modules or **/node_modules in .gitignore style, or "**/node_modules": true in vscode setting style ("search.exclude" or "files.exclude"). If we use ..., should this be .../node_modules/...? That is odd.

This is more about file paths, rather than package paths, and is more about editor settings, rather than go build setting. I wonder why we want to invent our own wheel instead of already widely used glob patterns, .gitignore style settings, editor settings?

@adonovan
Copy link
Member

I think Dylan was concerned that ** isn't required to be a complete segment (e.g. x/**y) and I suggested ... since its existing use in the Go command does have that requirement. But if both .gitignore and vscode (and apparently the LSP protocol too) already support ** with the semantics we want, then we should definitely go with that. Sorry for confusing things.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/414317 mentions this issue: internal/lsp: Update FilterDisallow to support matching directories at arbitrary depth

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/417588 mentions this issue: internal/lsp: Add directoryFilters in import scanning

gopherbot pushed a commit to golang/tools that referenced this issue Jul 18, 2022
internal/lsp: Update FilterDisallow to support matching directories a…
dc45e74 
…t arbitrary depth.

In FilterDisallow, change filter to regex form to match with file paths. Add a unit regtest for FilterDisallow.

For golang/go#46438

Change-Id: I7de1986c1cb1b65844828fa618b72b1e6b76b5b9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414317
Run-TryBot: Dylan Le <dungtuanle@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@findleyr findleyr modified the milestones: gopls/later, gopls/v0.9.2 Jul 28, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/420959 mentions this issue: internal/lsp: use directoryFilters in import scanning

@cespare
Copy link
Contributor Author

cespare commented Aug 4, 2022

@dle8 Thanks very much for your work on this!

Should there be a doc update reflecting the change?

Should we change the default value of directoryFilters from ["-node_modules"] to ["-**/node_modules"]?

@findleyr
Copy link
Member

findleyr commented Aug 4, 2022

@cespare raises good points, thanks.

IMO, the answer to both questions is "yes" :)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/422356 mentions this issue: internal/lsp: update documentation for directoryFilters setting and default value

gopherbot pushed a commit to golang/tools that referenced this issue Aug 9, 2022
internal/lsp: update documentation for directoryFilters setting and d…
6fa767d 
…efault value

Add `**` usage to directoryFilters documentation. Change directoryFilters default value to `-**/node_modules`

For golang/go#46438

Change-Id: I3ea14ad8a20893d19df4cf8d584a7c7f9b213aab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/422356
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
rinchsan pushed a commit to rinchsan/gosimports that referenced this issue Aug 14, 2022
@findleyr @rinchsan
internal/lsp: use directoryFilters in import scanning
01cad11 
Based on dle8's CL 414454, wire in directoryFilters into the goimports
ModuleResolver scan. A few changes/fixes/additions from that CL:

- Fix a bug in filter validation that was causing -**/a not to validate.
- Fix a bug in regex building where -a was not treated as an excluded
  prefix (it was matching 'a' anywhere).
- Use absolute paths in the SkipPathInScan, so that we can evaluate
  directory filters relative to the workspace folder.
- Add several regression tests.
- Consolidate directoryFilters regression tests under a new
  directoryfilters_test.go file.
- Add several TODOs.

Fixes golang/go#46438

Change-Id: I55cd3d6410905216cc8cfbdf528f301d67c2bbac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420959
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Dylan Le <dungtuanle@google.com>
@golang golang locked and limited conversation to collaborators Aug 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@dle8 dle8

Labels
FeatureRequest FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.9.2
Development

No branches or pull requests
9 participants
@cespare @myitcv @hyangah @adonovan @stamblerre @gopherbot @heschi @dle8 @findleyr