From c77660547be6c4fd6488c9e58171f26455407aa7 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Fri, 3 Jan 2025 10:08:10 +0000 Subject: [PATCH] Rework error and debug handling - Do not silence Cobra errors - Remove stdlib log use - Make debug logging consistent with legacy CLI format - Consistently use Cobra command output writers, rather than bypassing them with color.Error --- cmd/platform/main.go | 11 ++++--- commands/completion.go | 13 ++++---- commands/list.go | 30 ++++--------------- commands/root.go | 62 +++++++++++++++++++++++---------------- internal/legacy/legacy.go | 25 ++++++++-------- 5 files changed, 67 insertions(+), 74 deletions(-) diff --git a/cmd/platform/main.go b/cmd/platform/main.go index 085d7a5..143a08f 100644 --- a/cmd/platform/main.go +++ b/cmd/platform/main.go @@ -1,11 +1,10 @@ package main import ( - "log" + "fmt" "os" "strings" - "github.com/fatih/color" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -14,16 +13,16 @@ import ( ) func main() { - log.SetOutput(color.Error) - // Load configuration. cnfYAML, err := config.LoadYAML() if err != nil { - log.Fatal(err) + fmt.Fprintln(os.Stderr, err) + os.Exit(1) } cnf, err := config.FromYAML(cnfYAML) if err != nil { - log.Fatal(err) + fmt.Fprintln(os.Stderr, err) + os.Exit(1) } // When Cobra starts, load Viper config from the environment. diff --git a/commands/completion.go b/commands/completion.go index 8ab8ae2..02dc0b9 100644 --- a/commands/completion.go +++ b/commands/completion.go @@ -9,7 +9,6 @@ import ( "path" "strings" - "github.com/fatih/color" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -19,9 +18,10 @@ import ( func newCompletionCommand(cnf *config.Config) *cobra.Command { return &cobra.Command{ - Use: "completion", - Short: "Print the completion script for your shell", - Args: cobra.MaximumNArgs(1), + Use: "completion", + Short: "Print the completion script for your shell", + Args: cobra.MaximumNArgs(1), + SilenceErrors: true, Run: func(cmd *cobra.Command, args []string) { completionArgs := []string{"_completion", "-g", "--program", cnf.Application.Executable} if len(args) > 0 { @@ -33,6 +33,7 @@ func newCompletionCommand(cnf *config.Config) *cobra.Command { Version: version, CustomPharPath: viper.GetString("phar-path"), Debug: viper.GetBool("debug"), + DebugLogFunc: debugLog, DisableInteraction: viper.GetBool("no-interaction"), Stdout: &b, Stderr: cmd.ErrOrStderr(), @@ -40,13 +41,13 @@ func newCompletionCommand(cnf *config.Config) *cobra.Command { } if err := c.Init(); err != nil { - debugLog("%s\n", color.RedString(err.Error())) + debugLog(err.Error()) os.Exit(1) return } if err := c.Exec(cmd.Context(), completionArgs...); err != nil { - debugLog("%s\n", color.RedString(err.Error())) + debugLog(err.Error()) exitCode := 1 var execErr *exec.ExitError if errors.As(err, &execErr) { diff --git a/commands/list.go b/commands/list.go index b3446ff..6835a37 100644 --- a/commands/list.go +++ b/commands/list.go @@ -3,12 +3,8 @@ package commands import ( "bytes" "encoding/json" - "errors" "fmt" - "os" - "os/exec" - "github.com/fatih/color" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -28,14 +24,14 @@ func newListCommand(cnf *config.Config) *cobra.Command { Version: version, CustomPharPath: viper.GetString("phar-path"), Debug: viper.GetBool("debug"), + DebugLogFunc: debugLog, DisableInteraction: viper.GetBool("no-interaction"), Stdout: &b, Stderr: cmd.ErrOrStderr(), Stdin: cmd.InOrStdin(), } if err := c.Init(); err != nil { - debugLog("%s\n", color.RedString(err.Error())) - os.Exit(1) + exitWithError(cmd, err) return } @@ -47,20 +43,13 @@ func newListCommand(cnf *config.Config) *cobra.Command { arguments = append(arguments, args[0]) } if err := c.Exec(cmd.Context(), arguments...); err != nil { - debugLog("%s\n", color.RedString(err.Error())) - exitCode := 1 - var execErr *exec.ExitError - if errors.As(err, &execErr) { - exitCode = execErr.ExitCode() - } - os.Exit(exitCode) + exitWithError(cmd, err) return } var list List if err := json.Unmarshal(b.Bytes(), &list); err != nil { - debugLog("%s\n", color.RedString(err.Error())) - os.Exit(1) + exitWithError(cmd, err) return } @@ -99,21 +88,14 @@ func newListCommand(cnf *config.Config) *cobra.Command { c.Stdout = cmd.OutOrStdout() arguments := []string{"list", "--format=" + format} if err := c.Exec(cmd.Context(), arguments...); err != nil { - debugLog("%s\n", color.RedString(err.Error())) - exitCode := 1 - var execErr *exec.ExitError - if errors.As(err, &execErr) { - exitCode = execErr.ExitCode() - } - os.Exit(exitCode) + exitWithError(cmd, err) } return } result, err := formatter.Format(&list, config.FromContext(cmd.Context())) if err != nil { - debugLog("%s\n", color.RedString(err.Error())) - os.Exit(1) + exitWithError(cmd, err) return } diff --git a/commands/root.go b/commands/root.go index 3d4a7b2..96afac4 100644 --- a/commands/root.go +++ b/commands/root.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io" - "log" "os" "os/exec" "path/filepath" @@ -51,11 +50,19 @@ func newRootCommand(cnf *config.Config, assets *vendorization.VendorAssets) *cob DisableFlagParsing: false, FParseErrWhitelist: cobra.FParseErrWhitelist{UnknownFlags: true}, SilenceUsage: true, - SilenceErrors: true, + SilenceErrors: false, PersistentPreRun: func(cmd *cobra.Command, _ []string) { if viper.GetBool("quiet") && !viper.GetBool("debug") && !viper.GetBool("verbose") { viper.Set("no-interaction", true) cmd.SetErr(io.Discard) + } else { + // Ensure the command's output writers can handle colors. + if cmd.OutOrStdout() == os.Stdout { + cmd.SetOut(color.Output) + } + if cmd.ErrOrStderr() == os.Stderr { + cmd.SetErr(color.Error) + } } if viper.GetBool("version") { versionCommand.Run(cmd, []string{}) @@ -74,32 +81,26 @@ func newRootCommand(cnf *config.Config, assets *vendorization.VendorAssets) *cob Version: version, CustomPharPath: viper.GetString("phar-path"), Debug: viper.GetBool("debug"), + DebugLogFunc: debugLog, DisableInteraction: viper.GetBool("no-interaction"), Stdout: cmd.OutOrStdout(), Stderr: cmd.ErrOrStderr(), Stdin: cmd.InOrStdin(), } if err := c.Init(); err != nil { - log.Println(color.RedString(err.Error())) - os.Exit(1) + exitWithError(cmd, err) return } if err := c.Exec(cmd.Context(), os.Args[1:]...); err != nil { - debugLog("%s\n", color.RedString(err.Error())) - exitCode := 1 - var execErr *exec.ExitError - if errors.As(err, &execErr) { - exitCode = execErr.ExitCode() - } - os.Exit(exitCode) + exitWithError(cmd, err) } }, - PersistentPostRun: func(_ *cobra.Command, _ []string) { - checkShellConfigLeftovers(cnf) + PersistentPostRun: func(cmd *cobra.Command, _ []string) { + checkShellConfigLeftovers(cmd.ErrOrStderr(), cnf) select { case rel := <-updateMessageChan: - printUpdateMessage(rel, cnf) + printUpdateMessage(cmd.ErrOrStderr(), rel, cnf) default: } }, @@ -160,7 +161,7 @@ func newRootCommand(cnf *config.Config, assets *vendorization.VendorAssets) *cob } // checkShellConfigLeftovers checks .zshrc and .bashrc for any leftovers from the legacy CLI -func checkShellConfigLeftovers(cnf *config.Config) { +func checkShellConfigLeftovers(w io.Writer, cnf *config.Config) { start := fmt.Sprintf("# BEGIN SNIPPET: %s configuration", cnf.Application.Name) end := "# END SNIPPET" shellConfigSnippet := regexp.MustCompile(regexp.QuoteMeta(start) + "(?s).+?" + regexp.QuoteMeta(end)) @@ -186,23 +187,23 @@ func checkShellConfigLeftovers(cnf *config.Config) { } if shellConfigSnippet.Match(shellConfig) { - fmt.Fprintf(color.Error, "%s Your %s file contains code that is no longer needed for the New %s\n", - color.YellowString("Warning:"), + fmt.Fprintf(w, "%s Your %s file contains code that is no longer needed for the New %s\n", + color.YellowString("Notice:"), shellConfigFile, cnf.Application.Name, ) - fmt.Fprintf(color.Error, "%s %s\n", color.YellowString("Please remove the following lines from:"), shellConfigFile) - fmt.Fprintf(color.Error, "\t%s\n", strings.ReplaceAll(string(shellConfigSnippet.Find(shellConfig)), "\n", "\n\t")) + fmt.Fprintf(w, "%s %s\n", color.YellowString("Please remove the following lines from:"), shellConfigFile) + fmt.Fprintf(w, "\t%s\n", strings.ReplaceAll(string(shellConfigSnippet.Find(shellConfig)), "\n", "\n\t")) } } } -func printUpdateMessage(newRelease *internal.ReleaseInfo, cnf *config.Config) { +func printUpdateMessage(w io.Writer, newRelease *internal.ReleaseInfo, cnf *config.Config) { if newRelease == nil { return } - fmt.Fprintf(color.Error, "\n\n%s %s → %s\n", + fmt.Fprintf(w, "\n\n%s %s → %s\n", color.YellowString(fmt.Sprintf("A new release of the %s is available:", cnf.Application.Name)), color.CyanString(version), color.CyanString(newRelease.Version), @@ -211,19 +212,19 @@ func printUpdateMessage(newRelease *internal.ReleaseInfo, cnf *config.Config) { executable, err := os.Executable() if err == nil && cnf.Wrapper.HomebrewTap != "" && isUnderHomebrew(executable) { fmt.Fprintf( - color.Error, + w, "To upgrade, run: brew update && brew upgrade %s\n", color.YellowString(cnf.Wrapper.HomebrewTap), ) } else if cnf.Wrapper.GitHubRepo != "" { fmt.Fprintf( - color.Error, + w, "To upgrade, follow the instructions at: https://github.com/%s#upgrade\n", cnf.Wrapper.GitHubRepo, ) } - fmt.Fprintf(color.Error, "%s\n\n", color.YellowString(newRelease.URL)) + fmt.Fprintf(w, "%s\n\n", color.YellowString(newRelease.URL)) } func isUnderHomebrew(binary string) bool { @@ -246,5 +247,16 @@ func debugLog(format string, v ...any) { return } - log.Printf(format, v...) + prefix := color.New(color.ReverseVideo).Sprintf("DEBUG") + fmt.Fprintf(color.Error, prefix+" "+strings.TrimSpace(format)+"\n", v...) +} + +func exitWithError(cmd *cobra.Command, err error) { + cmd.PrintErrln(color.RedString(err.Error())) + exitCode := 1 + var execErr *exec.ExitError + if errors.As(err, &execErr) { + exitCode = execErr.ExitCode() + } + os.Exit(exitCode) } diff --git a/internal/legacy/legacy.go b/internal/legacy/legacy.go index 3664cc9..b7a16bb 100644 --- a/internal/legacy/legacy.go +++ b/internal/legacy/legacy.go @@ -6,7 +6,6 @@ import ( _ "embed" "fmt" "io" - "log" "os" "os/exec" "path" @@ -77,6 +76,13 @@ type CLIWrapper struct { CustomPharPath string Debug bool DisableInteraction bool + DebugLogFunc func(string, ...any) +} + +func (c *CLIWrapper) debug(msg string, args ...any) { + if c.DebugLogFunc != nil { + c.DebugLogFunc(msg, args...) + } } func (c *CLIWrapper) cacheDir() string { @@ -86,7 +92,7 @@ func (c *CLIWrapper) cacheDir() string { // Init the CLI wrapper, creating a temporary directory and copying over files func (c *CLIWrapper) Init() error { if _, err := os.Stat(c.cacheDir()); os.IsNotExist(err) { - c.debugLog("cache directory does not exist, creating: %s", c.cacheDir()) + c.debug("Cache directory does not exist, creating: %s", c.cacheDir()) if err := os.Mkdir(c.cacheDir(), 0o700); err != nil { return fmt.Errorf("could not create temporary directory: %w", err) } @@ -95,7 +101,7 @@ func (c *CLIWrapper) Init() error { if err := fileLock.Lock(); err != nil { return fmt.Errorf("could not acquire lock: %w", err) } - c.debugLog("lock acquired: %s", fileLock.Path()) + c.debug("Lock acquired: %s", fileLock.Path()) //nolint:errcheck defer fileLock.Unlock() @@ -104,7 +110,7 @@ func (c *CLIWrapper) Init() error { return fmt.Errorf("legacy CLI phar file not found: %w", err) } - c.debugLog("phar file does not exist, copying: %s", c.PharPath()) + c.debug("Phar file does not exist, copying: %s", c.PharPath()) if err := copyFile(c.PharPath(), phar); err != nil { return fmt.Errorf("could not copy phar file: %w", err) } @@ -126,7 +132,7 @@ func (c *CLIWrapper) Init() error { } if _, err := os.Stat(c.PHPPath()); os.IsNotExist(err) { - c.debugLog("PHP binary does not exist, copying: %s", c.PHPPath()) + c.debug("PHP binary does not exist, copying: %s", c.PHPPath()) if err := c.copyPHP(); err != nil { return fmt.Errorf("could not copy files: %w", err) } @@ -183,7 +189,7 @@ func (c *CLIWrapper) Exec(ctx context.Context, args ...string) error { )) if err := cmd.Run(); err != nil { // Cleanup cache directory - c.debugLog("removing cache directory: %s", c.cacheDir()) + c.debug("Removing cache directory: %s", c.cacheDir()) os.RemoveAll(c.cacheDir()) return fmt.Errorf("could not run legacy CLI command: %w", err) } @@ -204,10 +210,3 @@ func (c *CLIWrapper) PharPath() string { func (c *CLIWrapper) ConfigPath() string { return path.Join(c.cacheDir(), "config.yaml") } - -// debugLog logs a debugging message, if debug is enabled -func (c *CLIWrapper) debugLog(msg string, v ...any) { - if c.Debug { - log.Printf(msg, v...) - } -}