Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Command for config handling #165

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

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.

cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
@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