refactor: Reorganize global CLI flags #2615
Conversation
nasdf
added
refactor
cli/config.go
Outdated
| "log-stacktrace": "log.stacktrace", | ||
| "log-source": "log.source", | ||
| "log-overrides": "log.overrides", | ||
| "log-no-color": "log.nocolor", |
There was a problem hiding this comment.
suggestion: Would be nice to have a consistent usage of no and disable terminology and the no prefix to disable a feature.
For example: To disable key ring and p2p users use: no-keyring and no-p2p respectively, i.e. leading no prefix
But for disabling log, it is log-no-color rather than no-log-color.
Now for config keys we use: keyring.disabled net.p2pdisabled (the disabled term)
But for log we have log.nocolor instead of log.colordisabled
IMO would be nice to have at least user facing flags to be predictable / consistent in terminology.
cli/config.go
Outdated
| @@ -138,6 +139,7 @@ func loadConfig(rootdir string, flags *pflag.FlagSet) (*viper.Viper, error) { | |||
| Output: cfg.GetString("log.output"), | |||
| EnableStackTrace: cfg.GetBool("log.stacktrace"), | |||
| EnableSource: cfg.GetBool("log.source"), | |||
| DisableColor: cfg.GetBool("log.nocolor"), | |||
There was a problem hiding this comment.
suggestion: The // set default logging config appears incorrect though - this is not setting the default, but instead takes from the CLI params? Suggest either changing the comment or removing it.
Relevant issue(s)
Resolves #2614
Resolves #2582
Description
This PR moves the global CLI flags that only apply to the start command. It also fixes the log color config issue above.
Tasks
How has this been tested?
make testSpecify the platform(s) on which this was tested: