From c049b8f87b174f93adcf1cabc56961e0801c190d Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Fri, 24 Mar 2023 17:37:19 +0000 Subject: [PATCH 1/8] WIP --- cli/root.go | 68 ++++++++++++++++------------------------------------- 1 file changed, 20 insertions(+), 48 deletions(-) diff --git a/cli/root.go b/cli/root.go index 9cb03a71f3885..f9ec8c20f8e2d 100644 --- a/cli/root.go +++ b/cli/root.go @@ -1,7 +1,6 @@ package cli import ( - "bufio" "context" "errors" "flag" @@ -19,9 +18,7 @@ import ( "strings" "syscall" "time" - "unicode/utf8" - "golang.org/x/crypto/ssh/terminal" "golang.org/x/exp/slices" "golang.org/x/xerrors" @@ -826,28 +823,11 @@ type prettyErrorFormatter struct { w io.Writer } -func (prettyErrorFormatter) prefixLines(spaces int, s string) string { - twidth, _, err := terminal.GetSize(0) - if err != nil { - twidth = 80 - } - - s = lipgloss.NewStyle().Width(twidth - spaces).Render(s) - - var b strings.Builder - scanner := bufio.NewScanner(strings.NewReader(s)) - for i := 0; scanner.Scan(); i++ { - // The first line is already padded. - if i == 0 { - _, _ = fmt.Fprintf(&b, "%s\n", scanner.Text()) - continue - } - _, _ = fmt.Fprintf(&b, "%s%s\n", strings.Repeat(" ", spaces), scanner.Text()) +func (p *prettyErrorFormatter) format(err error) { + if err == nil { + return } - return strings.TrimSuffix(strings.TrimSuffix(b.String(), "\n"), " ") -} -func (p *prettyErrorFormatter) format(err error) { underErr := errors.Unwrap(err) arrowStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#515151")) @@ -859,41 +839,33 @@ func (p *prettyErrorFormatter) format(err error) { return } - var ( - padding string - arrowWidth int - ) - if p.level > 0 { - const arrow = "┗━ " - arrowWidth = utf8.RuneCount([]byte(arrow)) - padding = strings.Repeat(" ", arrowWidth*p.level) - _, _ = fmt.Fprintf(p.w, "%v%v", padding, arrowStyle.Render(arrow)) - } - + errorWithoutChildren := err.Error() if underErr != nil { - header := strings.TrimSuffix(err.Error(), ": "+underErr.Error()) - _, _ = fmt.Fprintf(p.w, "%s\n", p.prefixLines(len(padding)+arrowWidth, header)) - p.level++ - p.format(underErr) - return + errorWithoutChildren = strings.TrimSuffix(err.Error(), ": "+underErr.Error()) } - { + // Format the root error specially. + if p.level == 0 { style := lipgloss.NewStyle().Foreground(lipgloss.Color("#D16644")).Background(lipgloss.Color("#000000")).Bold(false) // This is the last error in a tree. p.wrappedPrintf( "%s\n", - p.prefixLines( - len(padding)+arrowWidth, - fmt.Sprintf( - "%s%s%s", - lipgloss.NewStyle().Inherit(style).Underline(true).Render("ERROR"), - lipgloss.NewStyle().Inherit(style).Foreground(arrowStyle.GetForeground()).Render(" ► "), - style.Render(err.Error()), - ), + fmt.Sprintf( + "%s%s%s", + lipgloss.NewStyle().Inherit(style).Underline(true).Render("ERROR"), + lipgloss.NewStyle().Inherit(style).Foreground(arrowStyle.GetForeground()).Render(" ► "), + style.Render(errorWithoutChildren), ), ) + p.level++ + p.format(underErr) + return } + + _, _ = fmt.Fprintf(p.w, "%s\n", errorWithoutChildren) + p.level++ + p.format(underErr) + return } func (p *prettyErrorFormatter) wrappedPrintf(format string, a ...interface{}) { From 042cb1888f3f100bfde255bacda8165ebf07aa3e Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Fri, 24 Mar 2023 17:56:47 +0000 Subject: [PATCH 2/8] Looks good to me --- cli/root.go | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/cli/root.go b/cli/root.go index f9ec8c20f8e2d..82141c78c1313 100644 --- a/cli/root.go +++ b/cli/root.go @@ -830,8 +830,6 @@ func (p *prettyErrorFormatter) format(err error) { underErr := errors.Unwrap(err) - arrowStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#515151")) - //nolint:errorlint if _, ok := err.(*clibase.RunCommandError); ok && p.level == 0 && underErr != nil { // We can do a better job now. @@ -844,17 +842,16 @@ func (p *prettyErrorFormatter) format(err error) { errorWithoutChildren = strings.TrimSuffix(err.Error(), ": "+underErr.Error()) } - // Format the root error specially. + const errorHeader = "ERROR" + // Format the root error specially since it's the most relevant. if p.level == 0 { - style := lipgloss.NewStyle().Foreground(lipgloss.Color("#D16644")).Background(lipgloss.Color("#000000")).Bold(false) - // This is the last error in a tree. + textStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#D16644")).Bold(false) p.wrappedPrintf( "%s\n", fmt.Sprintf( - "%s%s%s", - lipgloss.NewStyle().Inherit(style).Underline(true).Render("ERROR"), - lipgloss.NewStyle().Inherit(style).Foreground(arrowStyle.GetForeground()).Render(" ► "), - style.Render(errorWithoutChildren), + "%s\n%s", + lipgloss.NewStyle().Inherit(textStyle).Underline(true).Render(errorHeader), + textStyle.Render(errorWithoutChildren), ), ) p.level++ @@ -862,10 +859,24 @@ func (p *prettyErrorFormatter) format(err error) { return } - _, _ = fmt.Fprintf(p.w, "%s\n", errorWithoutChildren) + textStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#969696")).Bold(false) + + border := strings.Repeat("─", len(errorHeader)) + p.wrappedPrintf( + "%s\n%s\n", + textStyle.Render(border), + textStyle.Render(errorWithoutChildren), + ) p.level++ + if underErr == nil { + // Print closing border. + p.wrappedPrintf( + "%s\n", + textStyle.Render(border), + ) + return + } p.format(underErr) - return } func (p *prettyErrorFormatter) wrappedPrintf(format string, a ...interface{}) { From 7ac6301f66d96860315d280e0837baf2ebdab4fd Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Fri, 24 Mar 2023 20:37:26 +0000 Subject: [PATCH 3/8] Get rid of decoration --- cli/root.go | 27 ++++++++++----------------- cli/root_test.go | 4 ++++ 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/cli/root.go b/cli/root.go index 82141c78c1313..8e2fd6d4099eb 100644 --- a/cli/root.go +++ b/cli/root.go @@ -842,18 +842,21 @@ func (p *prettyErrorFormatter) format(err error) { errorWithoutChildren = strings.TrimSuffix(err.Error(), ": "+underErr.Error()) } - const errorHeader = "ERROR" // Format the root error specially since it's the most relevant. if p.level == 0 { textStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#D16644")).Bold(false) + var msg string + var sdkError *codersdk.Error + if errors.As(err, &sdkError) { + msg = sdkError.Message + sdkError.Helper + } else { + msg = errorWithoutChildren + } p.wrappedPrintf( "%s\n", - fmt.Sprintf( - "%s\n%s", - lipgloss.NewStyle().Inherit(textStyle).Underline(true).Render(errorHeader), - textStyle.Render(errorWithoutChildren), - ), + textStyle.Render(msg), ) + p.level++ p.format(underErr) return @@ -861,21 +864,11 @@ func (p *prettyErrorFormatter) format(err error) { textStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#969696")).Bold(false) - border := strings.Repeat("─", len(errorHeader)) p.wrappedPrintf( - "%s\n%s\n", - textStyle.Render(border), + "%s", textStyle.Render(errorWithoutChildren), ) p.level++ - if underErr == nil { - // Print closing border. - p.wrappedPrintf( - "%s\n", - textStyle.Render(border), - ) - return - } p.format(underErr) } diff --git a/cli/root_test.go b/cli/root_test.go index 22c1c3e36ae85..6d343ac973dfb 100644 --- a/cli/root_test.go +++ b/cli/root_test.go @@ -42,6 +42,10 @@ func TestCommandHelp(t *testing.T) { cmd []string } tests := []testCase{ + { + name: "coder --help", + cmd: []string{"--help"}, + }, { name: "coder --help", cmd: []string{"--help"}, From 0612a27d791b346346b519f8228a80431d6787a3 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Fri, 24 Mar 2023 21:16:35 +0000 Subject: [PATCH 4/8] one line --- cli/root.go | 22 ++++++++-------------- cli/root_test.go | 4 ---- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/cli/root.go b/cli/root.go index 8e2fd6d4099eb..ac05441194f25 100644 --- a/cli/root.go +++ b/cli/root.go @@ -13,7 +13,6 @@ import ( "os" "os/signal" "path/filepath" - "regexp" "runtime" "strings" "syscall" @@ -825,6 +824,8 @@ type prettyErrorFormatter struct { func (p *prettyErrorFormatter) format(err error) { if err == nil { + // 🍒 + p.printf("\n") return } @@ -852,8 +853,8 @@ func (p *prettyErrorFormatter) format(err error) { } else { msg = errorWithoutChildren } - p.wrappedPrintf( - "%s\n", + p.printf( + "%s", textStyle.Render(msg), ) @@ -864,23 +865,16 @@ func (p *prettyErrorFormatter) format(err error) { textStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#969696")).Bold(false) - p.wrappedPrintf( + p.printf( "%s", - textStyle.Render(errorWithoutChildren), + textStyle.Render(": "+errorWithoutChildren), ) p.level++ p.format(underErr) } -func (p *prettyErrorFormatter) wrappedPrintf(format string, a ...interface{}) { - s := lipgloss.NewStyle().Width(ttyWidth()).Render( - fmt.Sprintf(format, a...), - ) - - // Not sure why, but lipgloss is adding extra spaces we need to remove. - excessSpaceRe := regexp.MustCompile(`[[:blank:]]*\n[[:blank:]]*$`) - s = excessSpaceRe.ReplaceAllString(s, "\n") - +func (p *prettyErrorFormatter) printf(format string, a ...interface{}) { + s := fmt.Sprintf(format, a...) _, _ = p.w.Write( []byte( s, diff --git a/cli/root_test.go b/cli/root_test.go index 6d343ac973dfb..22c1c3e36ae85 100644 --- a/cli/root_test.go +++ b/cli/root_test.go @@ -42,10 +42,6 @@ func TestCommandHelp(t *testing.T) { cmd []string } tests := []testCase{ - { - name: "coder --help", - cmd: []string{"--help"}, - }, { name: "coder --help", cmd: []string{"--help"}, From 97deb1bbc6618df179894d182cdb9fc4f74e4bb4 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Fri, 24 Mar 2023 21:23:12 +0000 Subject: [PATCH 5/8] Fix colons and clean up code --- cli/root.go | 53 +++++++++++++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/cli/root.go b/cli/root.go index ac05441194f25..9d2fde5f4a447 100644 --- a/cli/root.go +++ b/cli/root.go @@ -824,53 +824,50 @@ type prettyErrorFormatter struct { func (p *prettyErrorFormatter) format(err error) { if err == nil { - // 🍒 + // 🏁 p.printf("\n") return } - underErr := errors.Unwrap(err) + nextErr := errors.Unwrap(err) //nolint:errorlint - if _, ok := err.(*clibase.RunCommandError); ok && p.level == 0 && underErr != nil { - // We can do a better job now. - p.format(underErr) + if _, ok := err.(*clibase.RunCommandError); ok && p.level == 0 && nextErr != nil { + // Avoid extra nesting. + p.format(nextErr) return } - errorWithoutChildren := err.Error() - if underErr != nil { - errorWithoutChildren = strings.TrimSuffix(err.Error(), ": "+underErr.Error()) + var headErr string + if nextErr != nil { + headErr = strings.TrimSuffix(err.Error(), ": "+nextErr.Error()) + } else { + headErr = err.Error() } - // Format the root error specially since it's the most relevant. - if p.level == 0 { - textStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#D16644")).Bold(false) - var msg string - var sdkError *codersdk.Error - if errors.As(err, &sdkError) { - msg = sdkError.Message + sdkError.Helper - } else { - msg = errorWithoutChildren - } - p.printf( - "%s", - textStyle.Render(msg), - ) + var msg string + var sdkError *codersdk.Error + if errors.As(err, &sdkError) { + msg = sdkError.Message + sdkError.Helper + } else { + msg = headErr + } - p.level++ - p.format(underErr) - return + textStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#D16644")) + if p.level > 0 { + textStyle.Foreground(lipgloss.Color("#969696")) } - textStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#969696")).Bold(false) + if nextErr != nil { + msg = msg + ": " + } p.printf( "%s", - textStyle.Render(": "+errorWithoutChildren), + textStyle.Render(msg), ) p.level++ - p.format(underErr) + p.format(nextErr) } func (p *prettyErrorFormatter) printf(format string, a ...interface{}) { From aaf95d92c39962ce81d5189939795b7086af88da Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Fri, 24 Mar 2023 21:25:18 +0000 Subject: [PATCH 6/8] Add comment --- cli/root.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/root.go b/cli/root.go index 9d2fde5f4a447..521e76bc2c041 100644 --- a/cli/root.go +++ b/cli/root.go @@ -855,6 +855,7 @@ func (p *prettyErrorFormatter) format(err error) { textStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#D16644")) if p.level > 0 { + // Grey out the less important, deep errors. textStyle.Foreground(lipgloss.Color("#969696")) } From c9e31bb5df0d206f0b8026aad74287b50617d662 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Fri, 24 Mar 2023 22:03:50 +0000 Subject: [PATCH 7/8] Remove unnecessary recursion --- cli/root.go | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/cli/root.go b/cli/root.go index 521e76bc2c041..1c2255ea1e217 100644 --- a/cli/root.go +++ b/cli/root.go @@ -818,29 +818,22 @@ func isConnectionError(err error) bool { } type prettyErrorFormatter struct { - level int - w io.Writer + w io.Writer } func (p *prettyErrorFormatter) format(err error) { - if err == nil { - // 🏁 - p.printf("\n") - return - } - - nextErr := errors.Unwrap(err) + errTail := errors.Unwrap(err) //nolint:errorlint - if _, ok := err.(*clibase.RunCommandError); ok && p.level == 0 && nextErr != nil { + if _, ok := err.(*clibase.RunCommandError); ok && errTail != nil { // Avoid extra nesting. - p.format(nextErr) + p.format(errTail) return } var headErr string - if nextErr != nil { - headErr = strings.TrimSuffix(err.Error(), ": "+nextErr.Error()) + if errTail != nil { + headErr = strings.TrimSuffix(err.Error(), ": "+errTail.Error()) } else { headErr = err.Error() } @@ -848,27 +841,34 @@ func (p *prettyErrorFormatter) format(err error) { var msg string var sdkError *codersdk.Error if errors.As(err, &sdkError) { - msg = sdkError.Message + sdkError.Helper + // We don't want to repeat the same error message twice, so we + // only show the SDK error on the top of the stack. + msg = sdkError.Message + if sdkError.Helper != "" { + msg = msg + "\n" + sdkError.Helper + } + // The SDK error is usually good enough, and we don't want to overwhelm + // the user with output. + errTail = nil } else { msg = headErr } textStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#D16644")) - if p.level > 0 { - // Grey out the less important, deep errors. - textStyle.Foreground(lipgloss.Color("#969696")) - } - - if nextErr != nil { + if errTail != nil { msg = msg + ": " } - p.printf( "%s", textStyle.Render(msg), ) - p.level++ - p.format(nextErr) + + if errTail != nil { + // Grey out the less important, deep errors. + textStyle.Foreground(lipgloss.Color("#969696")) + p.printf("%s", textStyle.Render(errTail.Error())) + } + p.printf("\n") } func (p *prettyErrorFormatter) printf(format string, a ...interface{}) { From f8d5ecb6c2aed71650d486f11e2290cfe3b9cf57 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Fri, 24 Mar 2023 22:13:54 +0000 Subject: [PATCH 8/8] Cleanup styling --- cli/root.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/cli/root.go b/cli/root.go index 1c2255ea1e217..59096f4900bc7 100644 --- a/cli/root.go +++ b/cli/root.go @@ -854,25 +854,25 @@ func (p *prettyErrorFormatter) format(err error) { msg = headErr } - textStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#D16644")) - if errTail != nil { - msg = msg + ": " - } + headStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#D16644")) p.printf( + headStyle, "%s", - textStyle.Render(msg), + msg, ) + tailStyle := headStyle.Copy().Foreground(lipgloss.Color("#969696")) + if errTail != nil { + p.printf(headStyle, ": ") // Grey out the less important, deep errors. - textStyle.Foreground(lipgloss.Color("#969696")) - p.printf("%s", textStyle.Render(errTail.Error())) + p.printf(tailStyle, "%s", errTail.Error()) } - p.printf("\n") + p.printf(tailStyle, "\n") } -func (p *prettyErrorFormatter) printf(format string, a ...interface{}) { - s := fmt.Sprintf(format, a...) +func (p *prettyErrorFormatter) printf(style lipgloss.Style, format string, a ...interface{}) { + s := style.Render(fmt.Sprintf(format, a...)) _, _ = p.w.Write( []byte( s,