internal/lsp/source: factor out enum options pattern
Factor out processing of string enums when setting options results. In a
couple places this will result in errors being detected for invalid
values where they weren't before. Interestingly the default branch for
symbolMatcher was 'caseInsensitive', when in fact the default matcher in
the absence of any setting is 'fuzzy'. The command tests were implicitly
relying on this bug, passing 'default' to mean 'caseInsensitive'. Fix
this.
Also add a test, since option processing is not trivial.
Change-Id: Ib3ec4b1619add4c7315b6a689ca257e068da6027
Reviewed-on: https://go-review.googlesource.com/c/tools/+/258658
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Robert Findley <rfindley@google.com>
diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go
index e1c374c..c9f038f 100644
--- a/internal/lsp/cmd/cmd.go
+++ b/internal/lsp/cmd/cmd.go
@@ -263,7 +263,7 @@
var matcherString = map[source.SymbolMatcher]string{
source.SymbolFuzzy: "fuzzy",
source.SymbolCaseSensitive: "caseSensitive",
- source.SymbolCaseInsensitive: "default",
+ source.SymbolCaseInsensitive: "caseInsensitive",
}
func (c *connection) initialize(ctx context.Context, options func(*source.Options)) error {
diff --git a/internal/lsp/cmd/test/workspace_symbol.go b/internal/lsp/cmd/test/workspace_symbol.go
index 6b19e34..796dfb8 100644
--- a/internal/lsp/cmd/test/workspace_symbol.go
+++ b/internal/lsp/cmd/test/workspace_symbol.go
@@ -14,7 +14,7 @@
)
func (r *runner) WorkspaceSymbols(t *testing.T, query string, expectedSymbols []protocol.SymbolInformation, dirs map[string]struct{}) {
- r.runWorkspaceSymbols(t, "default", query, dirs)
+ r.runWorkspaceSymbols(t, "caseInsensitive", query, dirs)
}
func (r *runner) FuzzyWorkspaceSymbols(t *testing.T, query string, expectedSymbols []protocol.SymbolInformation, dirs map[string]struct{}) {
@@ -51,9 +51,11 @@
}
var workspaceSymbolsDir = map[string]string{
- "default": "",
- "fuzzy": "fuzzy",
- "caseSensitive": "casesensitive",
+ // TODO: make caseInsensitive test cases consistent with
+ // other matcher.
+ "caseInsensitive": "",
+ "fuzzy": "fuzzy",
+ "caseSensitive": "casesensitive",
}
func workspaceSymbolsGolden(matcher, query string) string {
diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go
index 653ec5e..615de2e 100644
--- a/internal/lsp/source/options.go
+++ b/internal/lsp/source/options.go
@@ -574,67 +574,41 @@
case "completionBudget":
result.setDuration(&o.CompletionBudget)
case "matcher":
- matcher, ok := result.asString()
- if !ok {
- break
- }
- switch strings.ToLower(matcher) {
- case "fuzzy":
- o.Matcher = Fuzzy
- case "casesensitive":
- o.Matcher = CaseSensitive
- default:
- o.Matcher = CaseInsensitive
+ if s, ok := result.asOneOf(
+ string(Fuzzy),
+ string(CaseSensitive),
+ string(CaseInsensitive),
+ ); ok {
+ o.Matcher = Matcher(s)
}
case "symbolMatcher":
- matcher, ok := result.asString()
- if !ok {
- break
- }
- switch strings.ToLower(matcher) {
- case "fuzzy":
- o.SymbolMatcher = SymbolFuzzy
- case "casesensitive":
- o.SymbolMatcher = SymbolCaseSensitive
- default:
- o.SymbolMatcher = SymbolCaseInsensitive
+ if s, ok := result.asOneOf(
+ string(SymbolFuzzy),
+ string(SymbolCaseInsensitive),
+ string(SymbolCaseSensitive),
+ ); ok {
+ o.SymbolMatcher = SymbolMatcher(s)
}
case "symbolStyle":
- style, ok := result.asString()
- if !ok {
- break
- }
- switch strings.ToLower(style) {
- case "full":
- o.SymbolStyle = FullyQualifiedSymbols
- case "dynamic":
- o.SymbolStyle = DynamicSymbols
- case "package":
- o.SymbolStyle = PackageQualifiedSymbols
- default:
- result.errorf("Unsupported symbol style %q", style)
+ if s, ok := result.asOneOf(
+ string(FullyQualifiedSymbols),
+ string(PackageQualifiedSymbols),
+ string(DynamicSymbols),
+ ); ok {
+ o.SymbolStyle = SymbolStyle(s)
}
case "hoverKind":
- hoverKind, ok := result.asString()
- if !ok {
- break
- }
- switch strings.ToLower(hoverKind) {
- case "nodocumentation":
- o.HoverKind = NoDocumentation
- case "singleline":
- o.HoverKind = SingleLine
- case "synopsisdocumentation":
- o.HoverKind = SynopsisDocumentation
- case "fulldocumentation":
- o.HoverKind = FullDocumentation
- case "structured":
- o.HoverKind = Structured
- default:
- result.errorf("Unsupported hover kind %q", hoverKind)
+ if s, ok := result.asOneOf(
+ string(NoDocumentation),
+ string(SingleLine),
+ string(SynopsisDocumentation),
+ string(FullDocumentation),
+ string(Structured),
+ ); ok {
+ o.HoverKind = HoverKind(s)
}
case "linkTarget":
@@ -644,15 +618,8 @@
result.setBool(&o.LinksInHover)
case "importShortcut":
- var s string
- result.setString(&s)
- switch strings.ToLower(s) {
- case "both":
- o.ImportShortcut = Both
- case "link":
- o.ImportShortcut = Link
- case "definition":
- o.ImportShortcut = Definition
+ if s, ok := result.asOneOf(string(Both), string(Link), string(Definition)); ok {
+ o.ImportShortcut = ImportShortcut(s)
}
case "analyses":
@@ -817,6 +784,21 @@
return b, true
}
+func (r *OptionResult) asOneOf(options ...string) (string, bool) {
+ s, ok := r.asString()
+ if !ok {
+ return "", false
+ }
+ lower := strings.ToLower(s)
+ for _, opt := range options {
+ if strings.ToLower(opt) == lower {
+ return opt, true
+ }
+ }
+ r.errorf("Invalid option %q for enum %q", r.Value, r.Name)
+ return "", false
+}
+
func (r *OptionResult) setString(s *string) {
if v, ok := r.asString(); ok {
*s = v
diff --git a/internal/lsp/source/options_test.go b/internal/lsp/source/options_test.go
new file mode 100644
index 0000000..0e686b6
--- /dev/null
+++ b/internal/lsp/source/options_test.go
@@ -0,0 +1,70 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package source
+
+import (
+ "testing"
+ "time"
+)
+
+func TestSetOption(t *testing.T) {
+ tests := []struct {
+ name string
+ value interface{}
+ wantError bool
+ check func(Options) bool
+ }{
+ {
+ name: "symbolStyle",
+ value: "dynamic",
+ check: func(o Options) bool { return o.SymbolStyle == DynamicSymbols },
+ },
+ {
+ name: "symbolStyle",
+ value: "",
+ wantError: true,
+ check: func(o Options) bool { return o.SymbolStyle == "" },
+ },
+ {
+ name: "symbolStyle",
+ value: false,
+ wantError: true,
+ check: func(o Options) bool { return o.SymbolStyle == "" },
+ },
+ {
+ name: "symbolMatcher",
+ value: "caseInsensitive",
+ check: func(o Options) bool { return o.SymbolMatcher == SymbolCaseInsensitive },
+ },
+ {
+ name: "completionBudget",
+ value: "2s",
+ check: func(o Options) bool { return o.CompletionBudget == 2*time.Second },
+ },
+ {
+ name: "staticcheck",
+ value: true,
+ check: func(o Options) bool { return o.Staticcheck == true },
+ },
+ {
+ name: "codelens",
+ value: map[string]interface{}{"generate": true},
+ check: func(o Options) bool { return o.Codelens["generate"] },
+ },
+ }
+
+ for _, test := range tests {
+ var opts Options
+ result := opts.set(test.name, test.value)
+ if (result.Error != nil) != test.wantError {
+ t.Fatalf("Options.set(%q, %v): result.Error = %v, want error: %t", test.name, test.value, result.Error, test.wantError)
+ }
+ // TODO: this could be made much better using cmp.Diff, if that becomes
+ // available in this module.
+ if !test.check(opts) {
+ t.Errorf("Options.set(%q, %v): unexpected result %+v", test.name, test.value, opts)
+ }
+ }
+}
