-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
This pull request has been linked to Shortcut Story #16500: feature: doctor: enable colours and emoji by default. |
internal/humanwriter/human.go
Outdated
printFunc = result.State.MustColor() | ||
} | ||
|
||
_, err = fmt.Fprint(w.out, printFunc("%s %s\n", prefix, result.Summary)) |
There was a problem hiding this comment.
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:
-
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.
-
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Jonathan Yu <jonathan@coder.com>
Main motivation for this it to make FAIL results (in red) really stand out. I find it an important usability enhancement.