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

Using persistent config file for execution #174

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ondrejfuhrer
Copy link
Collaborator

Description

In order to allow changing default configuration for user execution, the default config is stored in user home directory.
Any additional options passed to the command have preference.

Closes #156

Actions taken

  • storing default execution configuration in user home
  • adding option to ignore changes made to user config
  • updating README
  • fixing rubocop offenses

CC: @muescha

@ondrejfuhrer ondrejfuhrer force-pushed the persistent-config branch 3 times, most recently from 1f65e32 to 2c94561 Compare September 5, 2020 20:14
- storing default execution configuration in user home
- adding option to ignore changes made to user config
- updating README
- fixing rubocop offenses
@z0rc
Copy link

z0rc commented Apr 9, 2021

Please do not put configuration dotfiles directly into user home dir. XDG Base Dir Spec is more than a decade old, majority of cli apps use it, like git for example.

Configuration files should be placed into $XDG_CONFIG_HOME with fallback to $HOME/.config is var isn't set. Or if it's entirely macOS specific, use $HOME/Library/Application Support for this.

default_options.quiet = default_values["quiet"]
default_options.verbose = default_values["verbose"]
default_options.interactive = default_values["interactive"]
default_options.debug = default_values["debug"]
Copy link
Contributor

Choose a reason for hiding this comment

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

the value debug is not in default_values

Copy link
Contributor

Choose a reason for hiding this comment

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

should a loop over the hash and a default_options[option_name] = default_values[option_name] a little shorter and include all options from hash?

Copy link
Contributor

Choose a reason for hiding this comment

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

i see there you also can write when you don't need to exclude some keys:

default_options = OpenStruct.new(default_values)

Comment on lines +137 to +140
unless File.exist?(config_filename)
odebug "Config file doesn't exist, creating"
create_default_config_file config_filename
end
Copy link
Contributor

@muescha muescha Dec 23, 2022

Choose a reason for hiding this comment

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

I don't expect here as an user that a normal brew cu creates always the config file.

if there is no config then do not create one.

only if there is an command brew cu --create-config I would expect that a config will be created.

my suggestion: remove this part and create an new command - or leave the creation of this to the user.

options = (YAML.safe_load f.read).to_h
odebug "Configuration loaded", options
end
OpenStruct.new options
Copy link
Contributor

Choose a reason for hiding this comment

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

  • at this point i would filter the keys to only allow keys which are known

  • maybe show an error message when keys are unknown - for example if there is a typo


default_options = create_default_options
if File.exist?(config_filename)
odebug "Loading configuration from config file"
Copy link
Contributor

Choose a reason for hiding this comment

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

add config file name

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.

Add a config for default behaviour
3 participants