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.

Sign up for GitHub

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

Jump to bottom

timetrace config command #42

Open
dominikbraun opened this issue May 19, 2021 · 7 comments · May be fixed by #165
Open

timetrace config command #42

dominikbraun opened this issue May 19, 2021 · 7 comments · May be fixed by #165
Labels
documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed

Comments

@dominikbraun
Copy link
Owner

At the moment, to get and set configuration values, users have to create a config.yaml file inside their .timetrace directory and set the values manually.

It would be nice to have a timetrace config command with two sub-commands to do this:

  • timetrace config set <KEY> <VALUE>
  • timetrace config get <KEY>

This way, users wouldn't have to touch their configuration file anymore.
@dominikbraun dominikbraun added documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed labels May 19, 2021
@Eddie023
Copy link

Hi @dominikbraun, I would like to work on this issue.

@Eddie023
Copy link

@dominikbraun Few questions regarding this issue. Here is my approach of solving this issue. let me know what you think.

  1. Create a userConfig file in config/ dir and upon each timetrace config set <KEY> <VALUE> command upsert the KEY VALUE.
  2. If the userConfig file is available then add some functionality like merge-deep-left to override withuserConfig list.

Question:

  1. Shouldn't we give them the list of possible configurable keys? Something like timetrace config list which will return all the configurable keys and the current value. Let me know what you think. I am confused what are the configurable things currently available.

@dominikbraun
Copy link
Owner Author

dominikbraun commented May 22, 2021 •
edited
Loading

@Eddie023 Your approach is fine, but you don't even need to introduce a new userConfig file because there already is support for config.yaml:

timetrace/config/config.go

Lines 16 to 48 in 2ba16af

// FromFile reads a configuration file called config.yml and returns it as a
// Config instance. If no configuration file is found, nil and no error will be
// returned. The configuration must live in one of the following directories:
//
// - /etc/timetrace
// - $HOME/.timetrace
// - .
//
// In case multiple configuration files are found, the one in the most specific
// or "closest" directory will be preferred.
func FromFile() (*Config, error) {
viper.SetConfigName("config")
viper.SetConfigType("yaml")
viper.AddConfigPath("/etc/timetrace/")
viper.AddConfigPath("$HOME/.timetrace")
viper.AddConfigPath(".")
if err := viper.ReadInConfig(); err != nil {
if _, ok := err.(viper.ConfigFileNotFoundError); !ok {
return nil, err
}
}
var config Config
if err := viper.Unmarshal(&config); err != nil {
return nil, err
}
cached = &config
return cached, nil
}

Shouldn't we give them the list of possible configurable keys? Something like timetrace config list which will return all the configurable keys and the current value. Let me know what you think. I am confused what are the configurable things currently available.

Yes. I think that this could even be done using timetrace config get without an argument. You could simply display all fields of config.Config to do so. BTW, you can get the already parsed configuration using the Timetrace.Config() function.

@dominikbraun
Copy link
Owner Author

Hi @Eddie023! Any updates on this one? If you don't have the capacities to do this issue at the moment, just let me know.

@Eddie023
Copy link

Eddie023 commented Jun 9, 2021 •
edited
Loading

@dominikbraun Sorry got caught up with work so working only on the weekends. I will send you draft PR by this weekend and you can decide what to do.

@Eddie023
Copy link

@dominikbraun Sorry that I wasn't able to do this issue this weekend as well. Had some personal work pulling me. I don't want to extend this issue any longer so I will just unassign myself. Again, sorry for not getting this issue through.

@Eddie023 Eddie023 removed their assignment Jun 14, 2021
@dominikbraun
Copy link
Owner Author

No problem, thanks for the update! 👍

@muhmuhhum muhmuhhum linked a pull request Jul 17, 2021 that will close this issue
Draft
@dominikbraun dominikbraun linked a pull request Aug 1, 2021 that will close this issue
Command for config handling #165
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Command for config handling muhmuhhum/timetrace
2 participants
@Eddie023 @dominikbraun