Skip to content

feat(hooks): provide a single list of hooks on command creation #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
arikkfir merged 1 commit into from
Jul 7, 2024
Merged

feat(hooks): provide a single list of hooks on command creation #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ type Command struct {
// MustNew creates a new command using [New], but will panic if it returns an error.
//
//goland:noinspection GoUnusedExportedFunction
func MustNew(name, shortDescription, longDescription string, action Action, preRunHooks []PreRunHook, postRunHooks []PostRunHook, subCommands ...*Command) *Command {
cmd, err := New(name, shortDescription, longDescription, action, preRunHooks, postRunHooks, subCommands...)
func MustNew(name, shortDescription, longDescription string, action Action, hooks []any, subCommands ...*Command) *Command {
cmd, err := New(name, shortDescription, longDescription, action, hooks, subCommands...)
if err != nil {
panic(err)
}
Expand All @@ -89,13 +89,32 @@ func MustNew(name, shortDescription, longDescription string, action Action, preR

// New creates a new command with the given name, short & long descriptions, and the given executor. The executor object
// is also scanned for configuration structs via reflection.
func New(name, shortDescription, longDescription string, action Action, preRunHooks []PreRunHook, postRunHooks []PostRunHook, subCommands ...*Command) (*Command, error) {
func New(name, shortDescription, longDescription string, action Action, hooks []any, subCommands ...*Command) (*Command, error) {
if name == "" {
return nil, fmt.Errorf("%w: empty name", ErrInvalidCommand)
} else if shortDescription == "" {
return nil, fmt.Errorf("%w: empty short description", ErrInvalidCommand)
}

// Translate the any-based hooks list into pre-run and post-run hooks
// Fail on any hook that doesn't implement at least one of them
var preRunHooks []PreRunHook
var postRunHooks []PostRunHook
for i, hook := range hooks {
var pre, post bool
if preRunHook, ok := hook.(PreRunHook); ok {
preRunHooks = append(preRunHooks, preRunHook)
pre = true
}
if postRunHook, ok := hook.(PostRunHook); ok {
postRunHooks = append(postRunHooks, postRunHook)
post = true
}
if !pre && !post {
return nil, fmt.Errorf("%w: hook %d (%T) is neither a PreRunHook nor a PostRunHook", ErrInvalidCommand, i, hook)
}
}

// Create the command instance
cmd := &Command{
name: name,
Expand Down
66 changes: 31 additions & 35 deletions command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,19 @@ func TestNew(t *testing.T) {
testCases := map[string]testCase{
"empty name": {
commandFactory: func(t T, tc *testCase) (*Command, error) {
return New("", "short desc", "long desc", nil, nil, nil)
return New("", "short desc", "long desc", nil, nil)
},
expectedError: `^invalid command: empty name$`,
},
"empty short description": {
commandFactory: func(t T, tc *testCase) (*Command, error) {
return New("cmd", "", "long desc", nil, nil, nil)
return New("cmd", "", "long desc", nil, nil)
},
expectedError: `^invalid command: empty short description$`,
},
"no flags": {
commandFactory: func(t T, tc *testCase) (*Command, error) {
return New("cmd", "desc", "long desc", nil, nil, nil)
return New("cmd", "desc", "long desc", nil, nil)
},
expectedName: "cmd",
expectedShortDescription: "desc",
Expand All @@ -54,7 +54,6 @@ func TestNew(t *testing.T) {
MyFlag string `flag:"true"`
}{},
nil,
nil,
)
},
expectedFlagSet: &flagSet{
Expand Down Expand Up @@ -97,13 +96,13 @@ func TestNew(t *testing.T) {
func TestAddSubCommand(t *testing.T) {
t.Parallel()

root, err := New("root", "desc", "description", nil, nil, nil)
root, err := New("root", "desc", "description", nil, nil)
With(t).Verify(err).Will(BeNil()).OrFail()

sub1, err := New("sub1", "sub1 desc", "sub1 description", nil, nil, nil)
sub1, err := New("sub1", "sub1 desc", "sub1 description", nil, nil)
With(t).Verify(err).Will(BeNil()).OrFail()

sub2, err := New("sub2", "sub2 desc", "sub2 description", nil, nil, nil)
sub2, err := New("sub2", "sub2 desc", "sub2 description", nil, nil)
With(t).Verify(err).Will(BeNil()).OrFail()

With(t).Verify(root.AddSubCommand(sub1)).Will(BeNil()).OrFail()
Expand All @@ -124,10 +123,10 @@ func Test_inferCommandAndArgs(t *testing.T) {
testCases := map[string]testCase{
"No arguments": {
root: MustNew(
"root", "desc", "description", nil, nil, nil,
MustNew("sub1", "sub1 desc", "sub1 description", nil, nil, nil,
MustNew("sub2", "sub2 desc", "sub2 description", nil, nil, nil,
MustNew("sub3", "sub3 desc", "sub3 description", nil, nil, nil),
"root", "desc", "description", nil, nil,
MustNew("sub1", "sub1 desc", "sub1 description", nil, nil,
MustNew("sub2", "sub2 desc", "sub2 description", nil, nil,
MustNew("sub3", "sub3 desc", "sub3 description", nil, nil),
),
),
),
Expand All @@ -138,9 +137,9 @@ func Test_inferCommandAndArgs(t *testing.T) {
},
"Flags for root command": {
root: MustNew(
"root", "desc", "description", nil, nil, nil,
MustNew("sub1", "sub1 desc", "sub1 description", nil, nil, nil,
MustNew("sub2", "sub2 desc", "sub2 description", nil, nil, nil),
"root", "desc", "description", nil, nil,
MustNew("sub1", "sub1 desc", "sub1 description", nil, nil,
MustNew("sub2", "sub2 desc", "sub2 description", nil, nil),
),
),
args: strings.Split("-f1 -f2", " "),
Expand All @@ -150,9 +149,9 @@ func Test_inferCommandAndArgs(t *testing.T) {
},
"Flags and positionals for root command": {
root: MustNew(
"root", "desc", "description", nil, nil, nil,
MustNew("sub1", "sub1 desc", "sub1 description", nil, nil, nil,
MustNew("sub2", "sub2 desc", "sub2 description", nil, nil, nil),
"root", "desc", "description", nil, nil,
MustNew("sub1", "sub1 desc", "sub1 description", nil, nil,
MustNew("sub2", "sub2 desc", "sub2 description", nil, nil),
),
),
args: strings.Split("-f1 a -f2 b", " "),
Expand All @@ -162,9 +161,9 @@ func Test_inferCommandAndArgs(t *testing.T) {
},
"Flags and positionals for sub1 command": {
root: MustNew(
"root", "desc", "description", nil, nil, nil,
MustNew("sub1", "sub1 desc", "sub1 description", nil, nil, nil,
MustNew("sub2", "sub2 desc", "sub2 description", nil, nil, nil),
"root", "desc", "description", nil, nil,
MustNew("sub1", "sub1 desc", "sub1 description", nil, nil,
MustNew("sub2", "sub2 desc", "sub2 description", nil, nil),
),
),
args: strings.Split("-f1 sub1 -f2 a b", " "),
Expand All @@ -174,9 +173,9 @@ func Test_inferCommandAndArgs(t *testing.T) {
},
"Flags and positionals for sub2 command": {
root: MustNew(
"root", "desc", "description", nil, nil, nil,
MustNew("sub1", "sub1 desc", "sub1 description", nil, nil, nil,
MustNew("sub2", "sub2 desc", "sub2 description", nil, nil, nil),
"root", "desc", "description", nil, nil,
MustNew("sub1", "sub1 desc", "sub1 description", nil, nil,
MustNew("sub2", "sub2 desc", "sub2 description", nil, nil),
),
),
args: strings.Split("-f1 sub1 -f2 a b sub2 c", " "),
Expand All @@ -201,10 +200,10 @@ func Test_getFullName(t *testing.T) {
cmd *Command
expectedFullName string
}
sub3 := MustNew("sub3", "sub3 desc", "sub3 description", nil, nil, nil)
sub2 := MustNew("sub2", "sub2 desc", "sub2 description", nil, nil, nil, sub3)
sub1 := MustNew("sub1", "sub1 desc", "sub1 description", nil, nil, nil, sub2)
root := MustNew("root", "desc", "description", nil, nil, nil, sub1)
sub3 := MustNew("sub3", "sub3 desc", "sub3 description", nil, nil)
sub2 := MustNew("sub2", "sub2 desc", "sub2 description", nil, nil, sub3)
sub1 := MustNew("sub1", "sub1 desc", "sub1 description", nil, nil, sub2)
root := MustNew("root", "desc", "description", nil, nil, sub1)
testCases := map[string]testCase{
"root": {
cmd: root,
Expand Down Expand Up @@ -236,10 +235,10 @@ func Test_getChain(t *testing.T) {
cmd *Command
expectedChain []string
}
sub3 := MustNew("sub3", "sub3 desc", "sub3 description", nil, nil, nil)
sub2 := MustNew("sub2", "sub2 desc", "sub2 description", nil, nil, nil, sub3)
sub1 := MustNew("sub1", "sub1 desc", "sub1 description", nil, nil, nil, sub2)
root := MustNew("root", "desc", "description", nil, nil, nil, sub1)
sub3 := MustNew("sub3", "sub3 desc", "sub3 description", nil, nil)
sub2 := MustNew("sub2", "sub2 desc", "sub2 description", nil, nil, sub3)
sub1 := MustNew("sub1", "sub1 desc", "sub1 description", nil, nil, sub2)
root := MustNew("root", "desc", "description", nil, nil, sub1)
testCases := map[string]testCase{
"root": {
cmd: root,
Expand Down Expand Up @@ -282,7 +281,7 @@ func TestPrintHelp(t *testing.T) {
"no flags & no positionals": {
commandFactory: func(*testCase) *Command {
ligen := loremipsum.NewWithSeed(4321)
return MustNew("cmd", ligen.Sentence(), ligen.Sentences(2), nil, nil, nil)
return MustNew("cmd", ligen.Sentence(), ligen.Sentences(2), nil, nil)
},
expectedHelpUsageOutput: `
Usage: cmd [--help]
Expand Down Expand Up @@ -326,7 +325,6 @@ Flags:
Args []string `args:"true"`
}{},
nil,
nil,
)
},
expectedHelpUsageOutput: `
Expand Down Expand Up @@ -377,7 +375,6 @@ Flags:
Args []string `args:"true"`
}{},
nil,
nil,
MustNew(
"child1",
ligen.Sentence(),
Expand All @@ -388,7 +385,6 @@ Flags:
Args []string `args:"true"`
}{},
nil,
nil,
),
)
},
Expand Down
38 changes: 19 additions & 19 deletions execute_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,23 +69,23 @@ func TestExecute(t *testing.T) {

t.Run("command must be root", func(t *testing.T) {
ctx := context.Background()
child := MustNew("child", "desc", "long desc", nil, nil, nil)
_ = MustNew("root", "desc", "long desc", nil, nil, nil, child)
child := MustNew("child", "desc", "long desc", nil, nil)
_ = MustNew("root", "desc", "long desc", nil, nil, child)
b := &bytes.Buffer{}
With(t).Verify(Execute(ctx, b, child, nil, nil)).Will(EqualTo(ExitCodeError)).OrFail()
With(t).Verify(b).Will(Say(`^unsupported operation: command must be the root command$`)).OrFail()
})

t.Run("applies configuration", func(t *testing.T) {
ctx := context.Background()
cmd := MustNew("cmd", "desc", "long desc", &ActionWithConfig{}, nil, nil)
cmd := MustNew("cmd", "desc", "long desc", &ActionWithConfig{}, nil)
With(t).Verify(Execute(ctx, os.Stderr, cmd, []string{"--my-flag=V1"}, nil)).Will(EqualTo(ExitCodeSuccess)).OrFail()
With(t).Verify(cmd.action.(*ActionWithConfig).MyFlag).Will(EqualTo("V1")).OrFail()
})

t.Run("prints usage on CLI parse errors", func(t *testing.T) {
ctx := context.Background()
cmd := MustNew("cmd", "desc", "long desc", &ActionWithConfig{}, nil, nil)
cmd := MustNew("cmd", "desc", "long desc", &ActionWithConfig{}, nil)
b := &bytes.Buffer{}
With(t).Verify(Execute(ctx, b, cmd, []string{"--bad-flag=V1"}, nil)).Will(EqualTo(ExitCodeMisconfiguration)).OrFail()
With(t).Verify(cmd.action.(*ActionWithConfig).MyFlag).Will(BeEmpty()).OrFail()
Expand All @@ -94,7 +94,7 @@ func TestExecute(t *testing.T) {

t.Run("prints help on --help flag", func(t *testing.T) {
ctx := context.Background()
cmd := MustNew("cmd", "desc", "long desc", &ActionWithConfig{}, nil, nil)
cmd := MustNew("cmd", "desc", "long desc", &ActionWithConfig{}, nil)
b := &bytes.Buffer{}
With(t).Verify(Execute(ctx, b, cmd, []string{"--help"}, nil)).Will(EqualTo(ExitCodeSuccess)).OrFail()
With(t).Verify(b.String()).Will(EqualTo(`
Expand All @@ -115,9 +115,9 @@ Flags:

t.Run("preRun called for command chain", func(t *testing.T) {
ctx := context.Background()
sub2 := MustNew("sub2", "desc", "long desc", &ActionWithConfig{}, []PreRunHook{&PreRunHookWithConfig{}}, nil)
sub1 := MustNew("sub1", "desc", "long desc", nil, []PreRunHook{&PreRunHookWithConfig{}}, nil, sub2)
root := MustNew("cmd", "desc", "long desc", nil, []PreRunHook{&PreRunHookWithConfig{}}, nil, sub1)
sub2 := MustNew("sub2", "desc", "long desc", &ActionWithConfig{}, []any{&PreRunHookWithConfig{}})
sub1 := MustNew("sub1", "desc", "long desc", nil, []any{&PreRunHookWithConfig{}}, sub2)
root := MustNew("cmd", "desc", "long desc", nil, []any{&PreRunHookWithConfig{}}, sub1)
With(t).Verify(Execute(ctx, os.Stderr, root, []string{"sub1", "sub2"}, nil)).Will(EqualTo(ExitCodeSuccess)).OrFail()

rootPreRunHook := root.preRunHooks[0].(*PreRunHookWithConfig)
Expand All @@ -140,9 +140,9 @@ Flags:
passThroughPreHook := func() PreRunHook { return &PreRunHookWithConfig{} }

ctx := context.Background()
sub2 := MustNew("sub2", "desc", "long desc", &ActionWithConfig{}, []PreRunHook{passThroughPreHook()}, nil)
sub1 := MustNew("sub1", "desc", "long desc", nil, []PreRunHook{passThroughPreHook(), failingPreHook}, nil, sub2)
root := MustNew("cmd", "desc", "long desc", nil, []PreRunHook{passThroughPreHook()}, nil, sub1)
sub2 := MustNew("sub2", "desc", "long desc", &ActionWithConfig{}, []any{passThroughPreHook()})
sub1 := MustNew("sub1", "desc", "long desc", nil, []any{passThroughPreHook(), failingPreHook}, sub2)
root := MustNew("cmd", "desc", "long desc", nil, []any{passThroughPreHook()}, sub1)

rootPreRunHook := root.preRunHooks[0].(*PreRunHookWithConfig)
sub1PreRunHook := sub1.preRunHooks[0].(*PreRunHookWithConfig)
Expand All @@ -159,9 +159,9 @@ Flags:

t.Run("postRun called for command chain", func(t *testing.T) {
ctx := context.Background()
sub2 := MustNew("sub2", "desc", "long desc", &ActionWithConfig{}, nil, []PostRunHook{&PostRunHookWithConfig{}})
sub1 := MustNew("sub1", "desc", "long desc", nil, nil, []PostRunHook{&PostRunHookWithConfig{}}, sub2)
root := MustNew("cmd", "desc", "long desc", nil, nil, []PostRunHook{&PostRunHookWithConfig{}}, sub1)
sub2 := MustNew("sub2", "desc", "long desc", &ActionWithConfig{}, []any{&PostRunHookWithConfig{}})
sub1 := MustNew("sub1", "desc", "long desc", nil, []any{&PostRunHookWithConfig{}}, sub2)
root := MustNew("cmd", "desc", "long desc", nil, []any{&PostRunHookWithConfig{}}, sub1)

exitCode := Execute(ctx, os.Stderr, root, []string{"sub1", "sub2"}, nil)
With(t).Verify(exitCode).Will(EqualTo(ExitCodeSuccess)).OrFail()
Expand Down Expand Up @@ -194,9 +194,9 @@ Flags:
failingAction := &ActionWithConfig{TrackingAction: TrackingAction{errorToReturnOnCall: fmt.Errorf("failing action")}}

ctx := context.Background()
sub2 := MustNew("sub2", "desc", "long desc", failingAction, nil, []PostRunHook{failingPostHook()})
sub1 := MustNew("sub1", "desc", "long desc", nil, nil, []PostRunHook{passThroughPostHook()}, sub2)
root := MustNew("cmd", "desc", "long desc", nil, nil, []PostRunHook{passThroughPostHook()}, sub1)
sub2 := MustNew("sub2", "desc", "long desc", failingAction, []any{failingPostHook()})
sub1 := MustNew("sub1", "desc", "long desc", nil, []any{passThroughPostHook()}, sub2)
root := MustNew("cmd", "desc", "long desc", nil, []any{passThroughPostHook()}, sub1)

exitCode := Execute(ctx, os.Stderr, root, []string{"sub1", "sub2"}, nil)
With(t).Verify(exitCode).Will(EqualTo(ExitCodeError)).OrFail()
Expand Down Expand Up @@ -228,7 +228,7 @@ Flags:
}
action := &ActionWithRequiredFlag{}
ctx := context.Background()
root := MustNew("cmd", "desc", "long desc", action, nil, nil)
root := MustNew("cmd", "desc", "long desc", action, nil)

b := &bytes.Buffer{}
With(t).Verify(Execute(ctx, b, root, nil, nil)).Will(EqualTo(ExitCodeMisconfiguration)).OrFail()
Expand All @@ -245,7 +245,7 @@ Flags:
MyFlag: "abc",
}
ctx := context.Background()
root := MustNew("cmd", "desc", "long desc", action, nil, nil)
root := MustNew("cmd", "desc", "long desc", action, nil)

b := &bytes.Buffer{}
With(t).Verify(Execute(ctx, b, root, nil, nil)).Will(EqualTo(ExitCodeSuccess)).OrFail()
Expand Down
12 changes: 12 additions & 0 deletions flag_set_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -978,6 +978,18 @@ func TestFlagSetApply(t *testing.T) {
args: []string{"--my-field1=VVV1"},
expectedError: `^required flag is missing: --my-field2$`,
},
"optional string field is not required": {
config: &struct {
F1 string `desc:"Some desc."`
F2 string `required:"true" desc:"Some desc."`
}{F2: "v2"},
envVars: map[string]string{},
args: []string{},
expectedConfig: &struct {
F1 string `desc:"Some desc."`
F2 string `required:"true" desc:"Some desc."`
}{F1: "", F2: "v2"},
},
"bool flag default value is considered": {
config: &struct {
F1 bool `name:"my-field1" required:"true"`
Expand Down