Initial implementation of 'console' package for stylized & localized console output 😂 #3638
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tstromberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
size/XXL
cmd/minikube/main.go
Outdated
| console.SetErrFile(os.Stderr) | ||
| err := console.SetPreferredLanguage(os.Getenv("LANG")) | ||
| if err != nil { | ||
| glog.Warningf("unable to detect language: %v", err) |
There was a problem hiding this comment.
can the user do anything about this? what does it mean? are we falling back to default English? it would be nice to clarify the implications to the user.
There was a problem hiding this comment.
Small nitpick here is that LANG is the locale (while LANGUAGE is the language)
LANG=sv_SE.UTF-8
LANGUAGE=sv
There was a problem hiding this comment.
LANG settings are incredibly complicated: LANG, LANGUAGE, and LC_* all control specific locale settings. LANG is the fallback.
To reduce complexity, I've removed the call to SetPreferredLanguage here. I'll re-introduce it once we have a way of loading alternative language strings.
pkg/minikube/console/console.go
Outdated
| func wantsColor(fd uintptr) bool { | ||
| glog.Infof("%s=%q\n", OverrideEnv, os.Getenv(OverrideEnv)) | ||
| switch os.Getenv(OverrideEnv) { | ||
| case "0": |
There was a problem hiding this comment.
I would consider using "true/false" or "on/off" or "yes/no" over "0/1"
There was a problem hiding this comment.
Normally you set the env var to enable, and leave it unset to disable.
In Go this means "" and "1", unless you want to bother with LookupEnv ?
There was a problem hiding this comment.
Since the idea here is to enable the user to opt out of this, and enable it by default...
Maybe we should change the variable name altogether ? like MINIKUBE_NOEMOJIS
Or keep the current approach, and use a string (not "bool") - and let "" mean auto-determine.
Just have to add some more case statements, to cover the above alternative spellings.
There was a problem hiding this comment.
I've switched this code to use strconv.ParseBool to allow true/false as well as 0/1. NOTE: Other parts of minikube use the ladder.
I've added some comments as well, to clarify the behavior. I want users to also be able to force color on, if our automatic sniffing isn't appropriate for their environment.
| // useColor is whether or not color output should be used, updated by Set*Writer. | ||
| useColor = false | ||
| // OverrideEnv is the environment variable used to override color/emoji usage | ||
| OverrideEnv = "MINIKUBE_IN_COLOR" |
There was a problem hiding this comment.
this should be a const rather
There was a problem hiding this comment.
I intentionally left this as a var, in case someone else wants to abuse this library for their own nefarious uses.
|
|
||
| // SetPreferredLanguage configures which language future messages should use, based on a LANG string. | ||
| func SetPreferredLanguage(s string) error { | ||
| if s == "" || s == "C" { |
There was a problem hiding this comment.
It's normally what you get, when you don't have any language support installed (including "en")
There was a problem hiding this comment.
Added a comment describing what C is about. It's hard to find an authoritative declaration of where the value originated, but it's well used and defined in POSIX: http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap07.html#tag_07_02
pkg/minikube/console/console.go
Outdated
| SetPreferredLanguageTag(defaultLanguage) | ||
| return nil | ||
| } | ||
| // Ignore encoding preferences: we always output utf8. Handles "de_DE.utf8" |
There was a problem hiding this comment.
do we want to warn about when we discard encoding preferences?
There was a problem hiding this comment.
Clarified comment. LANG settings which use non-utf8 encodings are fairly exotic, and unsupported in general nowadays. The world as a whole has moved to utf8 for console encodings*
- The Windows story is slightly more complicated, but Windows also doesn't support overriding the encoding format.
|
I think you meant "In front of our users 😀" - thanks for making it optional, though |
No need to sling objects around. You can today register a translation from anywhere via: err := message.SetString(language.Swedish, "Installing Kubernetes version %s ...", "Installerar Kubernetes version %s ...") Any future outputs that match the string will be translated appropriately. The intent though is to store a translation maps in a data file (JSON? YAML? Go?) that minikube can reference. Any string which does not have a translation value will default to the closest language -- or english. This behavior is asserted by console_test. https://phraseapp.com/blog/posts/internationalization-i18n-go/ has more info on this approach. |
|
LGTM, thank you! |
|
🎆 🍾 |
Introduces new console package, and replaces most calls to fmt.Print* with the appropriate calls to console.Out* (stdout) or console.Err* (stderr).
Example usage:
These messages are plumbed through to the message.Printer API, which provides a mechanism for future localization. Full support for styles is currently only enabled for TERM's which mention "color", and only then if it's writing to a TTY. This whitelist does not currently include the Windows console, as that will require further testing.
It is possible to manually turn styles off and on by setting MINIKUBE_IN_COLOR=[01] in your environment.
This resolves #3569 and paves the way for the following roadmap items:
My apologies for touching so many files in a single PR. This was due to heavy testing to make sure that the error messages plumbed to the user were proper enough for an initial implementation.