Remove pflag and rely on our own flag set (#61)

TODO.md

@@ -2,7 +2,7 @@
 
 Things I want to do to make this library as good as it can be and a better, simpler, more intuitive alternative to e.g. Cobra:
 
-- [ ] Replace `pflag.FlagSet` with my own implementation including parsing
+- [x] Replace `pflag.FlagSet` with my own implementation including parsing
 - [x] Make sure `cli.Option` is hidden and a `Command` can only be modified prior to building
 - [x] Ensure/double check that `cli.Option` applications are order independent
 - [x] Do some validation and maybe return an error from `cli.New` (TBC, might not need it)
@@ -15,6 +15,7 @@ Things I want to do to make this library as good as it can be and a better, simp
 - [ ] More full test programs as integration tests
 - [ ] Write a package example doc
 - [ ] Make the help output as pretty as possible, see [clap] for inspiration as their help is so nice
-- [ ] Implement something nice to do the whole `-- <bonus args>` thing, maybe `ExtraArgs`?
+- [x] Implement something nice to do the whole `-- <bonus args>` thing, maybe `ExtraArgs`?
+- [ ] Thin wrapper around tabwriter to keep it consistent
 
 [clap]: https://github.com/clap-rs/clap

command.go

@@ -9,15 +9,15 @@ import (
 	"slices"
 	"strings"
 	"text/tabwriter"
+	"unicode/utf8"
 
 	"github.com/FollowTheProcess/cli/internal/flag"
-	"github.com/spf13/pflag"
 )
 
 // TableWriter config, used for showing subcommands in help.
 const (
-	minWidth = 0    // Min cell width
-	tabWidth = 4    // Tab width in spaces
+	minWidth = 2    // Min cell width
+	tabWidth = 8    // Tab width in spaces
 	padding  = 1    // Padding
 	padChar  = '\t' // Char to pad with
 )
@@ -42,8 +42,7 @@ func New(name string, options ...Option) (*Command, error) {
 
 	// Default implementation
 	cfg := config{
-		flags:        pflag.NewFlagSet(name, pflag.ContinueOnError),
-		xflags:       flag.NewSet(),
+		flags:        flag.NewSet(),
 		stdin:        os.Stdin,
 		stdout:       os.Stdout,
 		stderr:       os.Stderr,
@@ -55,11 +54,19 @@ func New(name string, options ...Option) (*Command, error) {
 		argValidator: AnyArgs(),
 	}
 
+	// Ensure we always have at least help and version flags
+	defaultOptions := []Option{
+		Flag(&cfg.helpCalled, "help", 'h', false, fmt.Sprintf("Show help for %s", name)),
+		Flag(&cfg.versionCalled, "version", 'v', false, fmt.Sprintf("Show version info for %s", name)),
+	}
+
+	toApply := slices.Concat(options, defaultOptions)
+
 	// Apply the options, gathering up all the validation errors
 	// to report in one go. Each option returns only one error
 	// so this can be pre-allocated.
-	errs := make([]error, 0, len(options))
-	for _, option := range options {
+	errs := make([]error, 0, len(toApply))
+	for _, option := range toApply {
 		err := option.apply(&cfg)
 		if err != nil {
 			errs = append(errs, err)
@@ -97,10 +104,7 @@ type Command struct {
 	run func(cmd *Command, args []string) error
 
 	// flags is the set of flags for this command.
-	flags *pflag.FlagSet
-
-	// xflags is my experimental flagset.
-	xflags *flag.Set
+	flags *flag.Set
 
 	// versionFunc is the function thatgets called when the user calls -v/--version.
 	//
@@ -143,6 +147,12 @@ type Command struct {
 	//
 	// If the command has no subcommands, this slice will be nil.
 	subcommands []*Command
+
+	// helpCalled is whether or not the --help flag was used.
+	helpCalled bool
+
+	// versionCalled is whether or not the --version flag was used.
+	versionCalled bool
 }
 
 // example is a single usage example for a [Command].
@@ -189,22 +199,24 @@ func (c *Command) Execute() error {
 
 	// If -h/--help was called, call the defined helpFunc and exit so that
 	// the run function is never called.
-	helpCalled, err := cmd.flagSet().GetBool("help")
-	if err != nil {
-		return fmt.Errorf("could not parse help flag: %w", err)
+	helpCalled, ok := cmd.flagSet().Help()
+	if !ok {
+		// Should never get here as we define a default help
+		return errors.New("help flag not defined")
 	}
 	if helpCalled {
-		if err = defaultHelp(cmd); err != nil {
+		if err := defaultHelp(cmd); err != nil {
 			return fmt.Errorf("help function returned an error: %w", err)
 		}
 		return nil
 	}
 
 	// If -v/--version was called, call the defined versionFunc and exit so that
 	// the run function is never called
-	versionCalled, err := cmd.flagSet().GetBool("version")
-	if err != nil {
-		return fmt.Errorf("could not parse version flag: %w", err)
+	versionCalled, ok := cmd.flagSet().Version()
+	if !ok {
+		// Again, should be unreachable
+		return errors.New("version flag not defined")
 	}
 	if versionCalled {
 		if cmd.versionFunc == nil {
@@ -244,14 +256,14 @@ func (c *Command) Execute() error {
 }
 
 // Flags returns the set of flags for the command.
-func (c *Command) flagSet() *pflag.FlagSet {
+func (c *Command) flagSet() *flag.Set {
 	if c == nil {
 		// Only thing to do really, slightly more helpful than a generic
 		// nil pointer dereference
 		panic("Flags called on a nil Command")
 	}
 	if c.flags == nil {
-		return pflag.NewFlagSet(c.name, pflag.ContinueOnError)
+		return flag.NewSet()
 	}
 	return c.flags
 }
@@ -271,6 +283,14 @@ func (c *Command) Stdin() io.Reader {
 	return c.root().stdin
 }
 
+// ExtraArgs returns any additional arguments following a "--", this is useful for when you want to
+// implement argument pass through in your commands.
+//
+// If there were no extra arguments, it will return nil.
+func (c *Command) ExtraArgs() []string {
+	return c.flagSet().ExtraArgs()
+}
+
 // root returns the root of the command tree.
 func (c *Command) root() *Command {
 	if c.parent != nil {
@@ -281,24 +301,31 @@ func (c *Command) root() *Command {
 
 // hasFlag returns whether the command has a flag of the given name defined.
 func (c *Command) hasFlag(name string) bool {
-	flag := c.flagSet().Lookup(name)
-	if flag == nil {
+	if name == "" {
+		return false
+	}
+	flag, ok := c.flagSet().Get(name)
+	if !ok {
 		return false
 	}
-	return flag.NoOptDefVal != ""
+
+	return flag.DefaultValueNoArg != ""
 }
 
 // hasShortFlag returns whether the command has a shorthand flag of the given name defined.
 func (c *Command) hasShortFlag(name string) bool {
-	if len(name) == 0 {
+	if name == "" {
 		return false
 	}
 
-	flag := c.flagSet().ShorthandLookup(name[:1])
-	if flag == nil {
+	char, _ := utf8.DecodeRuneInString(name)
+
+	flag, ok := c.flagSet().GetShort(char)
+	if !ok {
 		return false
 	}
-	return flag.NoOptDefVal != ""
+
+	return flag.DefaultValueNoArg != ""
 }
 
 // findRequestedCommand uses the raw arguments and the command tree to determine what
@@ -395,6 +422,11 @@ func defaultHelp(cmd *Command) error {
 	if cmd == nil {
 		return errors.New("defaultHelp called on a nil Command")
 	}
+	usage, err := cmd.flagSet().Usage()
+	if err != nil {
+		return fmt.Errorf("could not write usage: %w", err)
+	}
+
 	// Note: The decision to not use text/template here is intentional, template calls
 	// reflect.Value.MethodByName() and/or reflect.Type.MethodByName() which disables dead
 	// code elimination in the compiler, meaning any application that uses cli for it's
@@ -460,7 +492,7 @@ func defaultHelp(cmd *Command) error {
 		s.WriteString("\n\n")
 	}
 	s.WriteString("Options:\n")
-	s.WriteString(cmd.flagSet().FlagUsages())
+	s.WriteString(usage)
 
 	// Subcommand help
 	s.WriteString("\n")

command_test.go

@@ -93,11 +93,12 @@ func TestSubCommandExecute(t *testing.T) {
 		stdout  string   // Expected stdout
 		stderr  string   // Expected stderr
 		args    []string // Args passed to root command
+		extra   []string // Extra args after "--" if present
 		wantErr bool     // Whether or not we wanted an error
 	}{
 		{
 			name:    "invoke sub1 no flags",
-			stdout:  "Hello from sub1, my args were: [my subcommand args], force was false, something was <empty>",
+			stdout:  "Hello from sub1, my args were: [my subcommand args], force was false, something was <empty>, extra args: []",
 			stderr:  "",
 			args:    []string{"sub1", "my", "subcommand", "args"},
 			wantErr: false,
@@ -111,28 +112,28 @@ func TestSubCommandExecute(t *testing.T) {
 		},
 		{
 			name:    "invoke sub1 with flags",
-			stdout:  "Hello from sub1, my args were: [my subcommand args], force was true, something was here",
+			stdout:  "Hello from sub1, my args were: [my subcommand args], force was true, something was here, extra args: []",
 			stderr:  "",
 			args:    []string{"sub1", "my", "subcommand", "args", "--force", "--something", "here"},
 			wantErr: false,
 		},
 		{
 			name:    "invoke sub1 with arg terminator",
-			stdout:  "Hello from sub1, my args were: [my subcommand args more args here], force was true, something was here",
+			stdout:  "Hello from sub1, my args were: [my subcommand args more args here], force was true, something was here, extra args: [more args here]",
 			stderr:  "",
 			args:    []string{"sub1", "my", "subcommand", "args", "--force", "--something", "here", "--", "more", "args", "here"},
 			wantErr: false,
 		},
 		{
 			name:    "invoke sub1 with sub1 in the arg list",
-			stdout:  "Hello from sub1, my args were: [my sub1 args sub1 more args here], force was true, something was here",
+			stdout:  "Hello from sub1, my args were: [my sub1 args sub1 more args here], force was true, something was here, extra args: []",
 			stderr:  "",
-			args:    []string{"sub1", "my", "sub1", "args", "sub1", "--force", "--something", "here", "--", "more", "args", "here"},
+			args:    []string{"sub1", "my", "sub1", "args", "sub1", "--force", "--something", "here", "more", "args", "here"},
 			wantErr: false,
 		},
 		{
 			name:    "invoke sub1 with sub1 as a flag value",
-			stdout:  "Hello from sub1, my args were: [my subcommand args more args here], force was true, something was sub2",
+			stdout:  "Hello from sub1, my args were: [my subcommand args more args here], force was true, something was sub2, extra args: []",
 			stderr:  "",
 			args:    []string{"sub1", "my", "subcommand", "args", "--force", "--something", "sub2", "more", "args", "here"},
 			wantErr: false,
@@ -165,10 +166,11 @@ func TestSubCommandExecute(t *testing.T) {
 					}
 					fmt.Fprintf(
 						cmd.Stdout(),
-						"Hello from sub1, my args were: %v, force was %v, something was %s",
+						"Hello from sub1, my args were: %v, force was %v, something was %s, extra args: %v",
 						args,
 						force,
 						something,
+						cmd.ExtraArgs(),
 					)
 					return nil
 				}),

go.mod

@@ -2,9 +2,6 @@ module github.com/FollowTheProcess/cli
 
 go 1.22
 
-require (
-	github.com/FollowTheProcess/test v0.11.1
-	github.com/spf13/pflag v1.0.5
-)
+require github.com/FollowTheProcess/test v0.11.1
 
 require github.com/google/go-cmp v0.6.0 // indirect

go.sum

@@ -2,5 +2,3 @@ github.com/FollowTheProcess/test v0.11.1 h1:f4peMzlID2+ZEbPzb4pITYLoicwqZc2J6iZF
 github.com/FollowTheProcess/test v0.11.1/go.mod h1:oIqlUoS8wKFmKBFBMJH/+asP7lQXE2D3YFhmUEubTUw=
 github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
 github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
-github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
-github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=

internal/flag/flag.go

@@ -23,8 +23,6 @@ import (
 	"unicode"
 	"unicode/utf8"
 	"unsafe"
-
-	"github.com/spf13/pflag"
 )
 
 const (
@@ -66,10 +64,7 @@ const (
 // should be the long hand version only e.g. --count, not -c/--count.
 const NoShortHand = rune(-1)
 
-var (
-	_ pflag.Value = Flag[string]{} // This will fail if we violate pflag.Value
-	_ Value       = Flag[string]{} // This one will fail if we violate our own Value interface
-)
+var _ Value = Flag[string]{} // This will fail if we violate our Value interface
 
 // Flaggable is a type constraint that defines any type capable of being parsed as a command line flag.
 //