review feedback p1

Author: ethanndickson

command.go

@@ -59,7 +59,7 @@ type Command struct {
 	Middleware  MiddlewareFunc
 	Handler     HandlerFunc
 	HelpHandler HandlerFunc
-	// CompletionHandler is called when the command is run is completion
+	// CompletionHandler is called when the command is run in completion
 	// mode. If nil, only the default completion handler is used.
 	//
 	// Flag and option parsing is best-effort in this mode, so even if an Option
@@ -187,7 +187,6 @@ func (c *Command) Invoke(args ...string) *Invocation {
 	return &Invocation{
 		Command: c,
 		Args:    args,
-		AllArgs: args,
 		Stdout:  io.Discard,
 		Stderr:  io.Discard,
 		Stdin:   strings.NewReader(""),
@@ -204,9 +203,6 @@ type Invocation struct {
 	// Args is reduced into the remaining arguments after parsing flags
 	// during Run.
 	Args []string
-	// AllArgs is the original arguments passed to the command, including flags.
-	// When invoked `WithOS`, this includes argv[0], otherwise it is the same as Args.
-	AllArgs []string
 	// CurWord is the word the terminal cursor is currently in
 	CurWord string
 
@@ -233,7 +229,6 @@ func (inv *Invocation) WithOS() *Invocation {
 		i.Stdout = os.Stdout
 		i.Stderr = os.Stderr
 		i.Stdin = os.Stdin
-		i.AllArgs = os.Args
 		i.Args = os.Args[1:]
 		i.Environ = ParseEnviron(os.Environ(), "")
 		i.Net = osNet{}
@@ -302,13 +297,13 @@ func copyFlagSetWithout(fs *pflag.FlagSet, without string) *pflag.FlagSet {
 	return fs2
 }
 
-func (inv *Invocation) GetCurWords() (prev string, cur string) {
-	if len(inv.AllArgs) == 1 {
-		cur = inv.AllArgs[0]
+func (inv *Invocation) curWords() (prev string, cur string) {
+	if len(inv.Args) == 1 {
+		cur = inv.Args[0]
 		prev = ""
 	} else {
-		cur = inv.AllArgs[len(inv.AllArgs)-1]
-		prev = inv.AllArgs[len(inv.AllArgs)-2]
+		cur = inv.Args[len(inv.Args)-1]
+		prev = inv.Args[len(inv.Args)-2]
 	}
 	return
 }
@@ -409,7 +404,39 @@ func (inv *Invocation) run(state *runState) error {
 		}
 	}
 
-	ignoreFlagParseErrors := inv.Command.RawArgs || inv.IsCompletionMode()
+	// 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() {
+		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 inv.equalsFlagHandler(cur) {
+				return nil
+			}
+		}
+		// 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 inv.flagHandler(prev) {
+				return nil
+			}
+		}
+		// If the current word is the command, auto-complete it so the shell moves the cursor
+		if inv.Command.Name() == inv.CurWord {
+			fmt.Fprintf(inv.Stdout, "%s\n", inv.Command.Name())
+			return nil
+		}
+		if inv.Command.CompletionHandler == nil {
+			inv.Command.CompletionHandler = DefaultCompletionHandler
+		}
+		for _, e := range inv.Command.CompletionHandler(inv) {
+			fmt.Fprintf(inv.Stdout, "%s\n", e)
+		}
+		return nil
+	}
+
+	ignoreFlagParseErrors := inv.Command.RawArgs
 
 	// Flag parse errors are irrelevant for raw args commands.
 	if !ignoreFlagParseErrors && state.flagParseErr != nil && !errors.Is(state.flagParseErr, pflag.ErrHelp) {
@@ -464,39 +491,6 @@ func (inv *Invocation) run(state *runState) error {
 	defer cancel()
 	inv = inv.WithContext(ctx)
 
-	// 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() {
-		prev, cur := inv.GetCurWords()
-		inv.CurWord = cur
-		// If the current word is a flag set using `=`, use it's handler
-		if strings.HasPrefix(cur, "--") && strings.Contains(cur, "=") {
-			if inv.equalsFlagHandler(cur) {
-				return nil
-			}
-		}
-		// 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 inv.flagHandler(prev) {
-				return nil
-			}
-		}
-		if inv.Command.Name() == inv.CurWord {
-			fmt.Fprintf(inv.Stdout, "%s\n", inv.Command.Name())
-			return nil
-		}
-		if inv.Command.CompletionHandler != nil {
-			for _, e := range inv.Command.CompletionHandler(inv) {
-				fmt.Fprintf(inv.Stdout, "%s\n", e)
-			}
-		}
-		for _, e := range DefaultCompletionHandler(inv) {
-			fmt.Fprintf(inv.Stdout, "%s\n", e)
-		}
-		return nil
-	}
-
 	if inv.Command.Handler == nil || errors.Is(state.flagParseErr, pflag.ErrHelp) {
 		if inv.Command.HelpHandler == nil {
 			return defaultHelpFn()(inv)
@@ -743,5 +737,3 @@ func RequireRangeArgs(start, end int) MiddlewareFunc {
 type HandlerFunc func(i *Invocation) error
 
 type CompletionHandlerFunc func(i *Invocation) []string
-
-var NopHandler HandlerFunc = func(i *Invocation) error { return nil }

command_test.go

@@ -31,7 +31,7 @@ func fakeIO(i *serpent.Invocation) *ioBufs {
 	return &b
 }
 
-func SampleCommand(t *testing.T) *serpent.Command {
+func sampleCommand(t *testing.T) *serpent.Command {
 	t.Helper()
 	var (
 		verbose bool
@@ -169,7 +169,7 @@ func SampleCommand(t *testing.T) *serpent.Command {
 func TestCommand(t *testing.T) {
 	t.Parallel()
 
-	cmd := func() *serpent.Command { return SampleCommand(t) }
+	cmd := func() *serpent.Command { return sampleCommand(t) }
 
 	t.Run("SimpleOK", func(t *testing.T) {
 		t.Parallel()

completion/all.go

@@ -20,17 +20,17 @@ const (
 )
 
 var shellCompletionByName = map[string]func(io.Writer, string) error{
-	BashShell:  GenerateCompletion(bashCompletionTemplate),
-	FishShell:  GenerateCompletion(fishCompletionTemplate),
-	ZShell:     GenerateCompletion(zshCompletionTemplate),
-	Powershell: GenerateCompletion(pshCompletionTemplate),
+	BashShell:  generateCompletion(bashCompletionTemplate),
+	FishShell:  generateCompletion(fishCompletionTemplate),
+	ZShell:     generateCompletion(zshCompletionTemplate),
+	Powershell: generateCompletion(pshCompletionTemplate),
 }
 
 func ShellOptions(choice *string) *serpent.Enum {
 	return serpent.EnumOf(choice, BashShell, FishShell, ZShell, Powershell)
 }
 
-func GetCompletion(writer io.Writer, shell string, cmdName string) error {
+func WriteCompletion(writer io.Writer, shell string, cmdName string) error {
 	fn, ok := shellCompletionByName[shell]
 	if !ok {
 		return fmt.Errorf("unknown shell %q", shell)
@@ -39,7 +39,7 @@ func GetCompletion(writer io.Writer, shell string, cmdName string) error {
 	return nil
 }
 
-func GetUserShell() (string, error) {
+func DetectUserShell() (string, error) {
 	// Attempt to get the SHELL environment variable first
 	if shell := os.Getenv("SHELL"); shell != "" {
 		return filepath.Base(shell), nil
@@ -70,7 +70,7 @@ func GetUserShell() (string, error) {
 	return "", fmt.Errorf("default shell not found")
 }
 
-func GenerateCompletion(
+func generateCompletion(
 	scriptTemplate string,
 ) func(io.Writer, string) error {
 	return func(w io.Writer, rootCmdName string) error {

completion/handlers.go

@@ -9,23 +9,24 @@ import (
 	"github.com/coder/serpent"
 )
 
-// FileHandler returns a handler that completes files, using the
+// FileHandler returns a handler that completes file names, using the
 // 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)
+		return listFiles(inv.CurWord, filter)
 	}
 }
 
+// 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)
+			return listFiles("", filter)
 		}
 		parts := strings.Split(curWord, ",")
-		out := ListFiles(parts[len(parts)-1], filter)
-		// prepend := strings.Join(parts[:len(parts)-1], ",")
+		out := listFiles(parts[len(parts)-1], filter)
 		for i, s := range out {
 			parts[len(parts)-1] = s
 			out[i] = strings.Join(parts, ",")
@@ -34,7 +35,7 @@ func FileListHandler(filter func(info os.FileInfo) bool) serpent.CompletionHandl
 	}
 }
 
-func ListFiles(word string, filter func(info os.FileInfo) bool) []string {
+func listFiles(word string, filter func(info os.FileInfo) bool) []string {
 	out := make([]string, 0, 32)
 
 	dir, _ := filepath.Split(word)

completion_test.go

@@ -13,7 +13,7 @@ import (
 func TestCompletion(t *testing.T) {
 	t.Parallel()
 
-	cmd := func() *serpent.Command { return SampleCommand(t) }
+	cmd := func() *serpent.Command { return sampleCommand(t) }
 
 	t.Run("SubcommandList", func(t *testing.T) {
 		t.Parallel()
@@ -100,7 +100,7 @@ func TestCompletion(t *testing.T) {
 func TestFileCompletion(t *testing.T) {
 	t.Parallel()
 
-	cmd := func() *serpent.Command { return SampleCommand(t) }
+	cmd := func() *serpent.Command { return sampleCommand(t) }
 
 	t.Run("DirOK", func(t *testing.T) {
 		t.Parallel()

example/completetest/main.go

@@ -8,10 +8,10 @@ import (
 	"github.com/coder/serpent/completion"
 )
 
-// InstallCommand returns a serpent command that helps
+// installCommand returns a serpent command that helps
 // a user configure their shell to use serpent's completion.
-func InstallCommand() *serpent.Command {
-	defaultShell, err := completion.GetUserShell()
+func installCommand() *serpent.Command {
+	defaultShell, err := completion.DetectUserShell()
 	if err != nil {
 		defaultShell = "bash"
 	}
@@ -21,7 +21,7 @@ func InstallCommand() *serpent.Command {
 		Use:   "completion",
 		Short: "Generate completion scripts for the given shell.",
 		Handler: func(inv *serpent.Invocation) error {
-			completion.GetCompletion(inv.Stdout, shell, inv.Command.Parent.Name())
+			completion.WriteCompletion(inv.Stdout, shell, inv.Command.Parent.Name())
 			return nil
 		},
 		Options: serpent.OptionSet{
@@ -116,7 +116,7 @@ func main() {
 				CompletionHandler: completion.FileHandler(nil),
 				Middleware:        serpent.RequireNArgs(1),
 			},
-			InstallCommand(),
+			installCommand(),
 		},
 	}