From 15270ec1c39d35e1437f36a2bf64aa9eacda73b1 Mon Sep 17 00:00:00 2001 From: Carlos Alexandro Becker Date: Fri, 15 Nov 2024 15:35:32 -0300 Subject: [PATCH 1/5] fix(confirm,choose,file,input): timeout handling - some fields were not actually using the `--timeout` value - some fields had different behavior when a timeout did occur. On this matter, it seems to me the best way forward is to specifically say it timed out, and after how long - added exit status 124 (copied from `timeout` from coreutils) (fixes #684) Signed-off-by: Carlos Alexandro Becker --- choose/command.go | 13 ++++++++-- confirm/command.go | 16 +++--------- confirm/command_test.go | 54 ----------------------------------------- file/command.go | 6 ++++- input/command.go | 6 +++++ internal/exit/exit.go | 30 +++++++++++++++++++++-- main.go | 4 +++ 7 files changed, 58 insertions(+), 71 deletions(-) delete mode 100644 confirm/command_test.go diff --git a/choose/command.go b/choose/command.go index f96b1beba..9d3d4508c 100644 --- a/choose/command.go +++ b/choose/command.go @@ -11,6 +11,7 @@ import ( "github.com/charmbracelet/x/ansi" "github.com/charmbracelet/x/term" + "github.com/charmbracelet/gum/internal/exit" "github.com/charmbracelet/gum/internal/stdin" ) @@ -76,8 +77,12 @@ func (o Options) Run() error { WithWidth(width). WithShowHelp(o.ShowHelp). WithTheme(theme). + WithTimeout(o.Timeout). Run() - if err != nil && !errors.Is(err, huh.ErrTimeout) { + if err != nil { + if errors.Is(err, huh.ErrTimeout) { + return exit.NewTimeout(o.Timeout) + } return err } if len(choices) > 0 { @@ -100,9 +105,13 @@ func (o Options) Run() error { ). WithWidth(width). WithTheme(theme). + WithTimeout(o.Timeout). WithShowHelp(o.ShowHelp). Run() - if err != nil && !errors.Is(err, huh.ErrTimeout) { + if err != nil { + if errors.Is(err, huh.ErrTimeout) { + return exit.NewTimeout(o.Timeout) + } return err } diff --git a/confirm/command.go b/confirm/command.go index 87f606583..18bcaac61 100644 --- a/confirm/command.go +++ b/confirm/command.go @@ -2,9 +2,9 @@ package confirm import ( "errors" - "fmt" "os" + "github.com/charmbracelet/gum/internal/exit" "github.com/charmbracelet/huh" ) @@ -31,11 +31,11 @@ func (o Options) Run() error { WithTheme(theme). WithShowHelp(o.ShowHelp). Run() - if err != nil { - if !o.errIsValidTimeout(err) { - return fmt.Errorf("unable to run confirm: %w", err) + if errors.Is(err, huh.ErrTimeout) { + return exit.NewTimeout(o.Timeout) } + return err } if !choice { @@ -44,11 +44,3 @@ func (o Options) Run() error { return nil } - -// errIsValidTimeout returns false unless 1) the user has specified a nonzero timeout and 2) the error is a huh.ErrTimeout. -func (o Options) errIsValidTimeout(err error) bool { - errWasTimeout := errors.Is(err, huh.ErrTimeout) - timeoutsExpected := o.Timeout > 0 - - return errWasTimeout && timeoutsExpected -} diff --git a/confirm/command_test.go b/confirm/command_test.go deleted file mode 100644 index 314a3b6d7..000000000 --- a/confirm/command_test.go +++ /dev/null @@ -1,54 +0,0 @@ -package confirm - -import ( - "fmt" - "github.com/charmbracelet/huh" - "testing" - "time" -) - -func TestOptions_errIsValidTimeout(t *testing.T) { - type testCase struct { - Description string - GivenTimeout time.Duration - GivenError error - ExpectedResult bool - } - - cases := []testCase{ - { - Description: "timeout is positive, err is a timeout", - GivenTimeout: time.Second, - GivenError: huh.ErrTimeout, - ExpectedResult: true, - }, - { - Description: "timeout is zero, err is a timeout", - GivenTimeout: 0, - GivenError: huh.ErrTimeout, - ExpectedResult: false, - }, - { - Description: "timeout is positive, err is not a timeout", - GivenTimeout: 1, - GivenError: fmt.Errorf("i'm not a timeout"), - ExpectedResult: false, - }, - { - Description: "timeout is zero, err is not a timeout", - GivenTimeout: 0, - GivenError: fmt.Errorf("i'm not a timeout"), - ExpectedResult: false, - }, - } - - for _, testCase := range cases { - t.Run(testCase.Description, func(t *testing.T) { - sut := Options{Timeout: testCase.GivenTimeout} - actualResult := sut.errIsValidTimeout(testCase.GivenError) - if actualResult != testCase.ExpectedResult { - t.Errorf("got: %v, want: %v", actualResult, testCase.ExpectedResult) - } - }) - } -} diff --git a/file/command.go b/file/command.go index f60ed2965..ed98547f9 100644 --- a/file/command.go +++ b/file/command.go @@ -5,6 +5,7 @@ import ( "fmt" "path/filepath" + "github.com/charmbracelet/gum/internal/exit" "github.com/charmbracelet/huh" "github.com/charmbracelet/lipgloss" ) @@ -49,12 +50,15 @@ func (o Options) Run() error { Value(&path), ), ). + WithTimeout(o.Timeout). WithShowHelp(o.ShowHelp). WithKeyMap(keymap). WithTheme(theme). Run() - if err != nil { + if errors.Is(err, huh.ErrTimeout) { + return exit.NewTimeout(o.Timeout) + } return err } diff --git a/input/command.go b/input/command.go index b35ce2084..81b9a80d8 100644 --- a/input/command.go +++ b/input/command.go @@ -1,6 +1,7 @@ package input import ( + "errors" "fmt" "os" @@ -9,6 +10,7 @@ import ( "github.com/charmbracelet/huh" "github.com/charmbracelet/lipgloss" + "github.com/charmbracelet/gum/internal/exit" "github.com/charmbracelet/gum/internal/stdin" ) @@ -53,10 +55,14 @@ func (o Options) Run() error { WithWidth(o.Width). WithTheme(theme). WithKeyMap(keymap). + WithTimeout(o.Timeout). WithShowHelp(o.ShowHelp). WithProgramOptions(tea.WithOutput(os.Stderr)). Run() if err != nil { + if errors.Is(err, huh.ErrTimeout) { + return exit.NewTimeout(o.Timeout) + } return err } diff --git a/internal/exit/exit.go b/internal/exit/exit.go index ff3a2b95c..204599d9e 100644 --- a/internal/exit/exit.go +++ b/internal/exit/exit.go @@ -1,9 +1,35 @@ package exit -import "fmt" +import ( + "fmt" + "time" + + "github.com/charmbracelet/huh" +) + +// StatusTimeout is the exit code for timed out commands. +const StatusTimeout = 124 // StatusAborted is the exit code for aborted commands. const StatusAborted = 130 -// ErrAborted is the error to return when a gum command is aborted by Ctrl + C. +// ErrAborted is the error to return when a gum command is aborted by Ctrl+C. var ErrAborted = fmt.Errorf("aborted") + +// NewTimeout returns a new ErrTimeout. +func NewTimeout(d time.Duration) ErrTimeout { + return ErrTimeout{d: d} +} + +// ErrTimeout is a time out error. +type ErrTimeout struct { + d time.Duration +} + +func (e ErrTimeout) Error() string { + return "timed out after " + e.d.String() +} + +func (e ErrTimeout) Unwrap() error { + return huh.ErrTimeout +} diff --git a/main.go b/main.go index b8f34076d..e1875babf 100644 --- a/main.go +++ b/main.go @@ -73,6 +73,10 @@ func main() { }, ) if err := ctx.Run(); err != nil { + if errors.Is(err, huh.ErrTimeout) { + fmt.Println(err) + os.Exit(exit.StatusTimeout) + } if errors.Is(err, exit.ErrAborted) || errors.Is(err, huh.ErrUserAborted) { os.Exit(exit.StatusAborted) } From 52e2237f560fc9825e9c99dae2d42e97fe08b552 Mon Sep 17 00:00:00 2001 From: Carlos Alexandro Becker Date: Fri, 15 Nov 2024 17:06:44 -0300 Subject: [PATCH 2/5] Update main.go Co-authored-by: Ayman Bagabas --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index e1875babf..9f748ff2a 100644 --- a/main.go +++ b/main.go @@ -74,7 +74,7 @@ func main() { ) if err := ctx.Run(); err != nil { if errors.Is(err, huh.ErrTimeout) { - fmt.Println(err) + fmt.Fprintln(os.Stderr, err) os.Exit(exit.StatusTimeout) } if errors.Is(err, exit.ErrAborted) || errors.Is(err, huh.ErrUserAborted) { From b14d8a81364dfa758989f17cb1dd82f13fe62f94 Mon Sep 17 00:00:00 2001 From: Carlos Alexandro Becker Date: Mon, 18 Nov 2024 09:16:19 -0300 Subject: [PATCH 3/5] Update internal/exit/exit.go Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com> --- internal/exit/exit.go | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/internal/exit/exit.go b/internal/exit/exit.go index 204599d9e..b1cc27304 100644 --- a/internal/exit/exit.go +++ b/internal/exit/exit.go @@ -18,18 +18,5 @@ var ErrAborted = fmt.Errorf("aborted") // NewTimeout returns a new ErrTimeout. func NewTimeout(d time.Duration) ErrTimeout { - return ErrTimeout{d: d} -} - -// ErrTimeout is a time out error. -type ErrTimeout struct { - d time.Duration -} - -func (e ErrTimeout) Error() string { - return "timed out after " + e.d.String() -} - -func (e ErrTimeout) Unwrap() error { - return huh.ErrTimeout + return fmt.Errorf("timed out after %s: %w", d, huh.ErrTimeout } From e6b624ce77b41ad0f2d588ad51cd2aa338c9ce3a Mon Sep 17 00:00:00 2001 From: Carlos Alexandro Becker Date: Mon, 18 Nov 2024 09:22:25 -0300 Subject: [PATCH 4/5] fix: improve Signed-off-by: Carlos Alexandro Becker --- choose/command.go | 10 ++-------- confirm/command.go | 6 +----- file/command.go | 6 +----- input/command.go | 6 +----- internal/exit/exit.go | 12 ++++++++---- main.go | 2 +- 6 files changed, 14 insertions(+), 28 deletions(-) diff --git a/choose/command.go b/choose/command.go index 9d3d4508c..d4b1c4219 100644 --- a/choose/command.go +++ b/choose/command.go @@ -80,10 +80,7 @@ func (o Options) Run() error { WithTimeout(o.Timeout). Run() if err != nil { - if errors.Is(err, huh.ErrTimeout) { - return exit.NewTimeout(o.Timeout) - } - return err + return exit.Handle(err, o.Timeout) } if len(choices) > 0 { s := strings.Join(choices, "\n") @@ -109,10 +106,7 @@ func (o Options) Run() error { WithShowHelp(o.ShowHelp). Run() if err != nil { - if errors.Is(err, huh.ErrTimeout) { - return exit.NewTimeout(o.Timeout) - } - return err + return exit.Handle(err, o.Timeout) } if term.IsTerminal(os.Stdout.Fd()) { diff --git a/confirm/command.go b/confirm/command.go index 18bcaac61..3653502d3 100644 --- a/confirm/command.go +++ b/confirm/command.go @@ -1,7 +1,6 @@ package confirm import ( - "errors" "os" "github.com/charmbracelet/gum/internal/exit" @@ -32,10 +31,7 @@ func (o Options) Run() error { WithShowHelp(o.ShowHelp). Run() if err != nil { - if errors.Is(err, huh.ErrTimeout) { - return exit.NewTimeout(o.Timeout) - } - return err + return exit.Handle(err, o.Timeout) } if !choice { diff --git a/file/command.go b/file/command.go index ed98547f9..c5bf71b1d 100644 --- a/file/command.go +++ b/file/command.go @@ -56,12 +56,8 @@ func (o Options) Run() error { WithTheme(theme). Run() if err != nil { - if errors.Is(err, huh.ErrTimeout) { - return exit.NewTimeout(o.Timeout) - } - return err + return exit.Handle(err, o.Timeout) } - fmt.Println(path) return nil } diff --git a/input/command.go b/input/command.go index 81b9a80d8..d35515eac 100644 --- a/input/command.go +++ b/input/command.go @@ -1,7 +1,6 @@ package input import ( - "errors" "fmt" "os" @@ -60,10 +59,7 @@ func (o Options) Run() error { WithProgramOptions(tea.WithOutput(os.Stderr)). Run() if err != nil { - if errors.Is(err, huh.ErrTimeout) { - return exit.NewTimeout(o.Timeout) - } - return err + return exit.Handle(err, o.Timeout) } fmt.Println(value) diff --git a/internal/exit/exit.go b/internal/exit/exit.go index b1cc27304..e79d7c22f 100644 --- a/internal/exit/exit.go +++ b/internal/exit/exit.go @@ -1,6 +1,7 @@ package exit import ( + "errors" "fmt" "time" @@ -14,9 +15,12 @@ const StatusTimeout = 124 const StatusAborted = 130 // ErrAborted is the error to return when a gum command is aborted by Ctrl+C. -var ErrAborted = fmt.Errorf("aborted") +var ErrAborted = huh.ErrUserAborted -// NewTimeout returns a new ErrTimeout. -func NewTimeout(d time.Duration) ErrTimeout { - return fmt.Errorf("timed out after %s: %w", d, huh.ErrTimeout +// Handle handles the error. +func Handle(err error, d time.Duration) error { + if errors.Is(err, huh.ErrTimeout) { + return fmt.Errorf("%w after %s", huh.ErrTimeout, d) + } + return err } diff --git a/main.go b/main.go index 9f748ff2a..5321fcdc3 100644 --- a/main.go +++ b/main.go @@ -77,7 +77,7 @@ func main() { fmt.Fprintln(os.Stderr, err) os.Exit(exit.StatusTimeout) } - if errors.Is(err, exit.ErrAborted) || errors.Is(err, huh.ErrUserAborted) { + if errors.Is(err, huh.ErrUserAborted) { os.Exit(exit.StatusAborted) } fmt.Println(err) From fbb0a65f0c50b865f0ca2ee20982d921881cc99e Mon Sep 17 00:00:00 2001 From: Carlos Alexandro Becker Date: Mon, 18 Nov 2024 09:23:36 -0300 Subject: [PATCH 5/5] fix: stderr --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index 5321fcdc3..301baefde 100644 --- a/main.go +++ b/main.go @@ -80,7 +80,7 @@ func main() { if errors.Is(err, huh.ErrUserAborted) { os.Exit(exit.StatusAborted) } - fmt.Println(err) + fmt.Fprintln(os.Stderr, err) os.Exit(1) } }