Command for config handling #165
Conversation
cli/config.go
Outdated
| Short: "Get config values", | ||
| Args: cobra.MaximumNArgs(1), | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| configKeys := make([][]string, 4) |
There was a problem hiding this comment.
This works and is perfectly valid code, but I always try to avoid having multiple places where something is hard-coded.
E.g. If a new Config option is added you have to add it here also. This is bad as it can be easily overlooked.
In this case you have two possible ways:
- Just generate json and display this (in a formatted way):
e.g.
{
"store": "",
"use12hours": false,
"editor": "",
"report-path": ""
}This would be easy to do using json.MarshalIndent
Downside:
It won't be formatted in the nice table-theme.
- Use reflection to load all public keys dynamically from the config struct
Not that easy, but fully flexible (json.Marshal uses that internally anyway)
@dominikbraun What do you think about this?
There was a problem hiding this comment.
@aligator @dominikbraun i have changed the get function to use reflection but I'm not to happy about the readability of the function. The other problem is that for now i only have bool and default as types supported by this function
There was a problem hiding this comment.
yes, I agree, reflection is not the most readable solution... (but in this case I still prefer it because of the reason I already wrote.)
-> that's why commenting the code should be done here. Just explain what the code does.
As for the not supported types: as the config won't be something very special and will only be composed of the default types, it is ok I think.
-> For now it is sufficient to just support string and bool as there is no other type currently. Error on any other type.
To alert the programmer if someone adds an unsupported type later, I would suggest to just error on any unsupported type and also to write a test which calls this reflection code. -> that test will then automatically fail if the config is no longer supported :-)
@dominikbraun @muhmuhhum
What do you think about this?
There was a problem hiding this comment.
Another Idea I just had, is:
Do it the previous way and write a test (which may use reflection) which checks if all fields of the config are covered by this comand.
Draft for #42
Adds commands to handle config settings.
First time writing go so feedback is appreciated