Skip to content
This repository was archived by the owner on Nov 14, 2024. It is now read-only.

feat: enable colored output #17

Merged
merged 4 commits into from
Sep 14, 2021
Merged

feat: enable colored output #17

merged 4 commits into from
Sep 14, 2021

Conversation

johnstcn
Copy link
Member

Main motivation for this it to make FAIL results (in red) really stand out. I find it an important usability enhancement.

@johnstcn johnstcn requested a review from jawnsy September 14, 2021 10:25
@johnstcn johnstcn self-assigned this Sep 14, 2021
@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #16500: feature: doctor: enable colours and emoji by default.

printFunc = result.State.MustColor()
}

_, err = fmt.Fprint(w.out, printFunc("%s %s\n", prefix, result.Summary))
Copy link
Contributor

@jawnsy jawnsy Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments on this one:

  1. Does this print the whole line in a given color? I think it'd be more readable if only the state name (WARN, INFO, etc) is color-coded. Of course, this does mean that emoji output wouldn't be colored, but I think that's OK, since the emoji itself should stand out enough for someone to know at-a-glance whether things are mostly passing or mostly failing.

    I'd suggest we check with Tyler (in lieu of a product designer, though we could also check with Jeb) or do a quick search for what other tools do. For example, Trivy only colors the status (WARN, etc), see screenshots in the README: https://github.com/aquasecurity/trivy

    Of all the comments, I feel most strongly about this one. But since it's also an easily-reversible change, I'm also fine with merging things as-is, and adjusting later if needed.

  2. Consider using Fprintln here. If you take the previous suggestion to avoid coloring the rest of the Summary line, then this also means the code can be simplified as:

    _, err = fmt.Fprintln(w.out, printFunc(prefix), result.Summary)

    In addition, it might make more sense to refactor the conditional like:

     if w.colors && w.mode == OutputModeText {
     	printFunc = result.State.MustColor()
     	_, err = fmt.Fprint(w.out, printFunc(prefix), result.Summary)
     }

    If we're only colorizing the prefix, then that text (with colors) could even be cached.

Copy link
Member Author

@johnstcn johnstcn Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the emoji itself should stand out enough for someone to know at-a-glance whether things are mostly passing or mostly failing.

I've found the difference between 👍 and 👎 is not extremely obvious.
Example:

👍 Coder 1.21.0 supports Kubernetes 1.19.0 to 1.22.0 (server version 1.20.2)
👍 found required resource:"serviceaccounts" group:"" version:"v1"
👎 missing required resource:"pods" group:"metrics.k8s.io" version:"metrics.k8s.io/v1beta1"
👍 found required resource:"roles" group:"rbac.authorization.k8s.io" version:"rbac.authorization.k8s.io/v1"
👍 found required resource:"storageclasses" group:"storage.k8s.io" version:"storage.k8s.io/v1"

If we just colorize the prefix, I'd recommend we change one or both of those emoji to be more visually distinct.
I propose ✅ and ❌ respectively. Here's what it looks like:

✅ found required resource:"services" group:"" version:"v1"
✅ found required resource:"deployments" group:"apps" version:"apps/v1"
❌ missing required resource:"pods" group:"metrics.k8s.io" version:"metrics.k8s.io/v1beta1"
✅ found required resource:"storageclasses" group:"storage.k8s.io" version:"storage.k8s.io/v1"

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if we use and from the Dingbats block, those appear to work with colours.
This seems to be what gotestsum uses.

Copy link
Member Author

@johnstcn johnstcn Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old:

image

New:
image

@johnstcn johnstcn merged commit d788ecd into main Sep 14, 2021
@johnstcn johnstcn deleted the cj/ch16500/pretty-colors branch September 14, 2021 17:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants