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.
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.
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.
Actually, if we use ✓ and ✗ from the Dingbats block, those appear to work with colours.
This seems to be what gotestsum uses.


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