Skip to content

Command for config handling#165

Draft
muhmuhhum wants to merge 3 commits intodominikbraun:mainfrom
muhmuhhum:main
Draft

Command for config handling#165
muhmuhhum wants to merge 3 commits intodominikbraun:mainfrom
muhmuhhum:main

Conversation

@muhmuhhum
Copy link

Draft for #42
Adds commands to handle config settings.
First time writing go so feedback is appreciated

@muhmuhhum muhmuhhum changed the title Add config get command Command for config handling Jul 17, 2021
Copy link
Collaborator

@aligator aligator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks,
just some comments and explanations :-)

cli/config.go Outdated
Short: "Get config values",
Args: cobra.MaximumNArgs(1),
Run: func(cmd *cobra.Command, args []string) {
configKeys := make([][]string, 4)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.

  1. 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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dominikbraun dominikbraun linked an issue Aug 1, 2021 that may be closed by this pull request
@dominikbraun dominikbraun added this to the timetrace v0.16.0 milestone Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

timetrace config command

3 participants