review

Author: ethanndickson

command.go

@@ -203,8 +203,6 @@ type Invocation struct {
 	// Args is reduced into the remaining arguments after parsing flags
 	// during Run.
 	Args []string
-	// CurWord is the word the terminal cursor is currently in
-	CurWord string
 
 	// Environ is a list of environment variables. Use EnvsWithPrefix to parse
 	// os.Environ.
@@ -297,7 +295,7 @@ func copyFlagSetWithout(fs *pflag.FlagSet, without string) *pflag.FlagSet {
 	return fs2
 }
 
-func (inv *Invocation) curWords() (prev string, cur string) {
+func (inv *Invocation) CurWords() (prev string, cur string) {
 	if len(inv.Args) == 1 {
 		cur = inv.Args[0]
 		prev = ""
@@ -407,7 +405,7 @@ func (inv *Invocation) run(state *runState) error {
 	// Outputted completions are not filtered based on the word under the cursor, as every shell we support does this already.
 	// We only look at the current word to figure out handler to run, or what directory to inspect.
 	if inv.IsCompletionMode() {
-		for _, e := range inv.doCompletions() {
+		for _, e := range inv.complete() {
 			fmt.Fprintln(inv.Stdout, e)
 		}
 		return nil
@@ -590,24 +588,36 @@ func (inv *Invocation) with(fn func(*Invocation)) *Invocation {
 	return &i2
 }
 
-func (inv *Invocation) doCompletions() []string {
-	prev, cur := inv.curWords()
-	inv.CurWord = cur
-	// If the current word is a flag set using `=`, use it's handler
-	if strings.HasPrefix(cur, "--") && strings.Contains(cur, "=") {
-		if out := inv.equalsFlagCompletions(cur); out != nil {
-			return out
+func (inv *Invocation) complete() []string {
+	prev, cur := inv.CurWords()
+
+	if strings.HasPrefix(cur, "--") {
+		// If the current word is a flag set using `=`, use it's handler
+		if strings.Contains(cur, "=") {
+			words := strings.Split(cur, "=")
+			flagName := words[0][2:]
+			if out := inv.completeFlag(flagName); out != nil {
+				for i, o := range out {
+					out[i] = fmt.Sprintf("--%s=%s", flagName, o)
+				}
+				return out
+			}
+		} else if out := inv.Command.Options.ByFlag(cur[2:]); out != nil {
+			// If the current word is a complete flag, auto-complete it so the
+			// shell moves the cursor
+			return []string{cur}
 		}
 	}
 	// If the previous word is a flag, then we're writing it's value
 	// and we should check it's handler
 	if strings.HasPrefix(prev, "--") {
-		if out := inv.flagCompletions(prev); out != nil {
+		word := prev[2:]
+		if out := inv.completeFlag(word); out != nil {
 			return out
 		}
 	}
-	// If the current word is the command, auto-complete it so the shell moves the cursor
-	if inv.Command.Name() == inv.CurWord {
+	// If the current word is the command, move the shell cursor
+	if inv.Command.Name() == cur {
 		return []string{inv.Command.Name()}
 	}
 	var completions []string
@@ -621,43 +631,17 @@ func (inv *Invocation) doCompletions() []string {
 	return completions
 }
 
-func (inv *Invocation) flagCompletions(word string) []string {
-	return inv.doFlagCompletions("", word)
-}
-
-func (inv *Invocation) equalsFlagCompletions(word string) []string {
-	words := strings.Split(word, "=")
-	word = words[0]
-	if len(words) > 1 {
-		inv.CurWord = words[1]
-	} else {
-		inv.CurWord = ""
-	}
-	prefix := word + "="
-	return inv.doFlagCompletions(prefix, word)
-}
-
-func (inv *Invocation) doFlagCompletions(prefix, word string) []string {
-	opt := inv.Command.Options.ByFlag(word[2:])
+func (inv *Invocation) completeFlag(word string) []string {
+	opt := inv.Command.Options.ByFlag(word)
 	if opt == nil {
 		return nil
 	}
 	if opt.CompletionHandler != nil {
-		completions := opt.CompletionHandler(inv)
-		out := make([]string, 0, len(completions))
-		for _, completion := range completions {
-			out = append(out, fmt.Sprintf("%s%s", prefix, completion))
-		}
-		return out
+		return opt.CompletionHandler(inv)
 	}
 	val, ok := opt.Value.(*Enum)
 	if ok {
-		completions := val.Choices
-		out := make([]string, 0, len(completions))
-		for _, choice := range completions {
-			out = append(out, fmt.Sprintf("%s%s", prefix, choice))
-		}
-		return out
+		return val.Choices
 	}
 	return nil
 }

command_test.go

@@ -154,11 +154,10 @@ func sampleCommand(t *testing.T) *serpent.Command {
 				},
 				Options: serpent.OptionSet{
 					{
-						Name:              "extra",
-						Flag:              "extra",
-						Description:       "Extra files.",
-						Value:             serpent.StringArrayOf(&fileArr),
-						CompletionHandler: completion.FileListHandler(nil),
+						Name:        "extra",
+						Flag:        "extra",
+						Description: "Extra files.",
+						Value:       serpent.StringArrayOf(&fileArr),
 					},
 				},
 				CompletionHandler: func(i *serpent.Invocation) []string {

completion/handlers.go

@@ -13,65 +13,43 @@ import (
 // given filter func, which may be nil.
 func FileHandler(filter func(info os.FileInfo) bool) serpent.CompletionHandlerFunc {
 	return func(inv *serpent.Invocation) []string {
-		return listFiles(inv.CurWord, filter)
-	}
-}
+		var out []string
+		_, word := inv.CurWords()
 
-// FileListHandler returns a handler that completes a list of comma-separated,
-// file names, using the given filter func, which may be nil.
-func FileListHandler(filter func(info os.FileInfo) bool) serpent.CompletionHandlerFunc {
-	return func(inv *serpent.Invocation) []string {
-		curWord := strings.TrimLeft(inv.CurWord, `"`)
-		if curWord == "" {
-			return listFiles("", filter)
+		dir, _ := filepath.Split(word)
+		if dir == "" {
+			dir = "."
 		}
-		parts := strings.Split(curWord, ",")
-		out := listFiles(parts[len(parts)-1], filter)
-		for i, s := range out {
-			parts[len(parts)-1] = s
-			out[i] = strings.Join(parts, ",")
+		f, err := os.Open(dir)
+		if err != nil {
+			return out
 		}
-		return out
-	}
-}
-
-func listFiles(word string, filter func(info os.FileInfo) bool) []string {
-	// Avoid reallocating for each of the first few files we see.
-	out := make([]string, 0, 16)
-
-	dir, _ := filepath.Split(word)
-	if dir == "" {
-		dir = "."
-	}
-	f, err := os.Open(dir)
-	if err != nil {
-		return out
-	}
-	defer f.Close()
-	if dir == "." {
-		dir = ""
-	}
-
-	infos, err := f.Readdir(0)
-	if err != nil {
-		return out
-	}
-
-	for _, info := range infos {
-		if filter != nil && !filter(info) {
-			continue
+		defer f.Close()
+		if dir == "." {
+			dir = ""
 		}
 
-		var cur string
-		if info.IsDir() {
-			cur = fmt.Sprintf("%s%s%c", dir, info.Name(), os.PathSeparator)
-		} else {
-			cur = fmt.Sprintf("%s%s", dir, info.Name())
+		infos, err := f.Readdir(0)
+		if err != nil {
+			return out
 		}
 
-		if strings.HasPrefix(cur, word) {
-			out = append(out, cur)
+		for _, info := range infos {
+			if filter != nil && !filter(info) {
+				continue
+			}
+
+			var cur string
+			if info.IsDir() {
+				cur = fmt.Sprintf("%s%s%c", dir, info.Name(), os.PathSeparator)
+			} else {
+				cur = fmt.Sprintf("%s%s", dir, info.Name())
+			}
+
+			if strings.HasPrefix(cur, word) {
+				out = append(out, cur)
+			}
 		}
+		return out
 	}
-	return out
 }

completion_test.go

@@ -85,6 +85,16 @@ func TestCompletion(t *testing.T) {
 		require.Equal(t, "--req-array\n--req-enum\n", io.Stdout.String())
 	})
 
+	t.Run("NoOptDefValueFlag", func(t *testing.T) {
+		t.Parallel()
+		i := cmd().Invoke("--verbose", "")
+		i.Environ.Set(serpent.CompletionModeEnv, "1")
+		io := fakeIO(i)
+		err := i.Run()
+		require.NoError(t, err)
+		require.Equal(t, "altfile\nfile\nrequired-flag\ntoupper\n--prefix\n", io.Stdout.String())
+	})
+
 	t.Run("EnumOK", func(t *testing.T) {
 		t.Parallel()
 		i := cmd().Invoke("required-flag", "--req-enum", "")
@@ -105,6 +115,16 @@ func TestCompletion(t *testing.T) {
 		require.Equal(t, "--req-enum=foo\n--req-enum=bar\n--req-enum=qux\n", io.Stdout.String())
 	})
 
+	t.Run("EnumEqualsBeginQuotesOK", func(t *testing.T) {
+		t.Parallel()
+		i := cmd().Invoke("required-flag", "--req-enum", "--req-enum=\"")
+		i.Environ.Set(serpent.CompletionModeEnv, "1")
+		io := fakeIO(i)
+		err := i.Run()
+		require.NoError(t, err)
+		require.Equal(t, "--req-enum=foo\n--req-enum=bar\n--req-enum=qux\n", io.Stdout.String())
+	})
+
 }
 
 func TestFileCompletion(t *testing.T) {
@@ -179,31 +199,5 @@ func TestFileCompletion(t *testing.T) {
 				require.Equal(t, len(files), len(output))
 			}
 		})
-		t.Run(tc.name+"/List", func(t *testing.T) {
-			t.Parallel()
-			for _, path := range tc.paths {
-				i := cmd().Invoke("altfile", "--extra", fmt.Sprintf(`"example.go,%s`, path))
-				i.Environ.Set(serpent.CompletionModeEnv, "1")
-				io := fakeIO(i)
-				err := i.Run()
-				require.NoError(t, err)
-				output := strings.Split(io.Stdout.String(), "\n")
-				output = output[:len(output)-1]
-				for _, str := range output {
-					parts := strings.Split(str, ",")
-					require.Len(t, parts, 2)
-					require.Equal(t, parts[0], "example.go")
-					fileComp := parts[1]
-					if strings.HasSuffix(fileComp, string(os.PathSeparator)) {
-						require.DirExists(t, fileComp)
-					} else {
-						require.FileExists(t, fileComp)
-					}
-				}
-				files, err := os.ReadDir(tc.realPath)
-				require.NoError(t, err)
-				require.Equal(t, len(files), len(output))
-			}
-		})
 	}
 }

example/completetest/main.go

@@ -108,9 +108,6 @@ func main() {
 						Flag:        "extra",
 						Description: "Extra files.",
 						Value:       serpent.StringArrayOf(&fileArr),
-						CompletionHandler: completion.FileListHandler(func(info os.FileInfo) bool {
-							return !info.IsDir()
-						}),
 					},
 				},
 				CompletionHandler: completion.FileHandler(nil),

option.go

@@ -347,6 +347,9 @@ func (optSet OptionSet) ByName(name string) *Option {
 }
 
 func (optSet OptionSet) ByFlag(flag string) *Option {
+	if flag == "" {
+		return nil
+	}
 	for i := range optSet {
 		opt := &optSet[i]
 		if opt.Flag == flag || opt.FlagShorthand == flag {