Repository: rsteube/carapace (go)

Issue:
performance: use references for internal action map
Use references as index skip uid generation and flag traversing in `init()` function to improve performance.
- calls to `FlagCompletion`, `PositionalCompletion` and `PositionalAnyCompletion` just store the argument
- needs check command to verify all configured flags exist (otherwise this won't be known at compile time)

```go
type actions struct {
  flag ActionMap
  positional []Action
  positionalAny Action
}

map[*cobra.Command]actions
```

Pull request:
use references for internal action map
fix #256 

Patch:
diff --git a/carapace.go b/carapace.go
index 6780d60..54e522d 100644
--- a/carapace.go
+++ b/carapace.go
@@ -24,8 +24,6 @@ import (
 	"github.com/spf13/pflag"
 )
 
-var actionMap ActionMap = make(ActionMap)
-
 type Carapace struct {
 	cmd *cobra.Command
 }
@@ -38,23 +36,15 @@ func Gen(cmd *cobra.Command) *Carapace {
 }
 
 func (c Carapace) PositionalCompletion(action ...Action) {
-	for index, a := range action {
-		actionMap[uid.Positional(c.cmd, index+1)] = a
-	}
+	storage.get(c.cmd).positional = action
 }
 
 func (c Carapace) PositionalAnyCompletion(action Action) {
-	actionMap[uid.Positional(c.cmd, 0)] = action
+	storage.get(c.cmd).positionalAny = action
 }
 
 func (c Carapace) FlagCompletion(actions ActionMap) {
-	for name, action := range actions {
-		if flag := c.cmd.LocalFlags().Lookup(name); flag == nil {
-			fmt.Fprintf(os.Stderr, "unknown flag: %v\n", name)
-		} else {
-			actionMap[uid.Flag(c.cmd, flag)] = action
-		}
-	}
+	storage.get(c.cmd).flag = actions
 }
 
 func (c Carapace) Standalone() {
@@ -126,7 +116,8 @@ func addCompletionCommand(cmd *cobra.Command) {
 				if len(args) == 1 {
 					switch args[0] {
 					case "debug":
-						for uid, action := range actionMap {
+						// TODO broken, not indexed by uid anymore
+						for uid, action := range storage {
 							fmt.Printf("%v:\t%v\n", uid, action)
 						}
 					default:
@@ -154,32 +145,31 @@ func addCompletionCommand(cmd *cobra.Command) {
 						// TODO needs more cleanup and tests
 						var targetAction Action
 						if flag := lookupFlag(targetCmd, previous); flag != nil && flag.NoOptDefVal == "" { // previous arg is a flag and needs a value
-							targetAction, _ = actionMap[uid.Flag(targetCmd, flag)]
+							targetAction = storage.getFlag(targetCmd, flag.Name)
 						} else if strings.HasPrefix(current, "-") { // assume flag
 							if strings.Contains(current, "=") { // complete value for optarg flag
 								if flag := lookupFlag(targetCmd, current); flag != nil && flag.NoOptDefVal != "" {
-									if a, ok := actionMap[uid.Flag(targetCmd, flag)]; ok {
-										// TODO no value for oil (elvish works)
-										splitted := strings.SplitN(current, "=", 2)
-										context.CallbackValue = splitted[1]
-										targetAction = a.Invoke(context).Prefix(splitted[0] + "=").ToA()
-									}
+									a := storage.getFlag(targetCmd, flag.Name)
+									// TODO no value for oil (elvish works)
+									splitted := strings.SplitN(current, "=", 2)
+									context.CallbackValue = splitted[1]
+									targetAction = a.Invoke(context).Prefix(splitted[0] + "=").ToA()
 								}
 							} else { // complete flagnames
 								targetAction = actionFlags(targetCmd)
 							}
 						} else if targetCmd.HasAvailableSubCommands() && len(targetArgs) <= 1 {
 							subcommandA := actionSubcommands(targetCmd)
-							if _, a, ok := findAction(targetCmd, targetArgs); ok {
-								subcommandA = a.Invoke(context).Merge(subcommandA.Invoke(context)).ToA()
-							}
+							a := findAction(targetCmd, targetArgs)
+							subcommandA = a.Invoke(context).Merge(subcommandA.Invoke(context)).ToA()
 							targetAction = subcommandA
 						} else {
-							_, targetAction, _ = findAction(targetCmd, targetArgs)
+							targetAction = findAction(targetCmd, targetArgs)
 						}
 						fmt.Fprintln(io.MultiWriter(os.Stdout, logger.Writer()), targetAction.Invoke(context).value(shell, context.CallbackValue))
 					default:
-						fmt.Fprintln(io.MultiWriter(os.Stdout, logger.Writer()), actionMap.invokeCallback(id, context).Invoke(context).value(shell, context.CallbackValue))
+						// TODO disable support for direct uid invocation
+						//fmt.Fprintln(io.MultiWriter(os.Stdout, logger.Writer()), actionMap.invokeCallback(id, context).Invoke(context).value(shell, context.CallbackValue))
 					}
 				}
 			}
@@ -191,22 +181,18 @@ func addCompletionCommand(cmd *cobra.Command) {
 	})
 }
 
-func findAction(targetCmd *cobra.Command, targetArgs []string) (id string, action Action, ok bool) {
+func findAction(targetCmd *cobra.Command, targetArgs []string) Action {
+	// TODO handle Action not found
 	if len(targetArgs) == 0 {
-		id = uid.Positional(targetCmd, 1)
+		return storage.getPositional(targetCmd, 0)
 	} else {
 		lastArg := targetArgs[len(targetArgs)-1]
-		if strings.HasSuffix(lastArg, " ") {
-			id = uid.Positional(targetCmd, len(targetArgs)+1)
+		if strings.HasSuffix(lastArg, " ") { // TODO is this still correct/needed?
+			return storage.getPositional(targetCmd, len(targetArgs))
 		} else {
-			id = uid.Positional(targetCmd, len(targetArgs))
+			return storage.getPositional(targetCmd, len(targetArgs)-1)
 		}
 	}
-	if action, ok = actionMap[id]; !ok {
-		id = uid.Positional(targetCmd, 0)
-		action, ok = actionMap[id]
-	}
-	return
 }
 
 func findTarget(cmd *cobra.Command, args []string) (*cobra.Command, []string, error) {
@@ -289,3 +275,17 @@ func initLogger() (err error) {
 	}
 	return
 }
+
+type testing interface {
+	Error(args ...interface{})
+}
+
+// Test verifies the configuration (e.g. flag name exists)
+//   func TestCarapace(t *testing.T) {
+//       carapace.Test(t)
+//   }
+func Test(t testing) {
+	for _, e := range storage.check() {
+		testing(t).Error(e)
+	}
+}
diff --git a/docs/src/carapace/gen.md b/docs/src/carapace/gen.md
index 3218d36..2a48370 100644
--- a/docs/src/carapace/gen.md
+++ b/docs/src/carapace/gen.md
@@ -11,3 +11,11 @@ carapace.Gen(myCmd)
 ```
 
 > Invocations to `carapace.Gen` must be **after** the command was added to the parent command so that the [uids](./gen/uid.md) are correct.
+
+Additionally invoke [`carapace.Test`](https://pkg.go.dev/github.com/rsteube/carapace#Test) in a [test](https://golang.org/doc/tutorial/add-a-test) to verify configuration during build time.
+```go
+func TestCarapace(t *testing.T) {
+    carapace.Test(t)
+}
+```
+
diff --git a/example/cmd/root.go b/example/cmd/root.go
index 6d3481e..5ac306b 100644
--- a/example/cmd/root.go
+++ b/example/cmd/root.go
@@ -22,6 +22,7 @@ func init() {
 	rootCmd.Flags().StringArrayP("array", "a", []string{}, "multiflag")
 
 	carapace.Gen(rootCmd).FlagCompletion(carapace.ActionMap{
-		"toggle": carapace.ActionBool(),
+		"toggle":         carapace.ActionBool(),
+		"persistentFlag": carapace.ActionValues("p1", "p2", "p3"),
 	})
 }
diff --git a/storage.go b/storage.go
new file mode 100644
index 0000000..3140897
--- /dev/null
+++ b/storage.go
@@ -0,0 +1,59 @@
+package carapace
+
+import (
+	"fmt"
+	"github.com/rsteube/carapace/internal/uid"
+	"github.com/spf13/cobra"
+)
+
+// TODO storage needs better naming and structure
+
+type entry struct {
+	flag          ActionMap
+	positional    []Action
+	positionalAny Action
+}
+
+type _storage map[*cobra.Command]*entry
+
+func (s _storage) get(cmd *cobra.Command) (e *entry) {
+	var ok bool
+	if e, ok = s[cmd]; !ok {
+		e = &entry{}
+		s[cmd] = e
+	}
+	return
+}
+
+func (s _storage) getFlag(cmd *cobra.Command, name string) Action {
+	if flag := cmd.LocalFlags().Lookup(name); flag == nil && cmd.HasParent() {
+		return s.getFlag(cmd.Parent(), name)
+	}
+	// TODO nil check?
+	return s.get(cmd).flag[name]
+}
+
+func (s _storage) getPositional(cmd *cobra.Command, pos int) Action {
+	entry := s.get(cmd)
+	// TODO nil check?
+	if len(entry.positional) > pos {
+		return entry.positional[pos]
+	} else {
+		return entry.positionalAny
+	}
+}
+
+// TODO implicit execution during build - go:generate possible?
+func (s _storage) check() []string {
+	errors := make([]string, 0)
+	for cmd, entry := range s {
+		for name := range entry.flag {
+			if flag := cmd.LocalFlags().Lookup(name); flag == nil {
+				errors = append(errors, fmt.Sprintf("unknown flag for %s: %s\n", uid.Command(cmd), name))
+			}
+		}
+	}
+	return errors
+}
+
+var storage = make(_storage)


Test patch:
diff --git a/example/main_test.go b/example/main_test.go
index 722e434..8fba8d6 100644
--- a/example/main_test.go
+++ b/example/main_test.go
@@ -6,6 +6,8 @@ import (
 	"os/exec"
 	"strings"
 	"testing"
+
+	"github.com/rsteube/carapace"
 )
 
 func doComplete(t *testing.T, shell string, cmdline string, contained ...string) {
@@ -176,3 +178,7 @@ func TestZsh(t *testing.T) {
 		doComplete(t, "zsh", cmdline, text)
 	}
 }
+
+func TestCarapace(t *testing.T) {
+	carapace.Test(t)
+}