Create default config if it does not exist#320
Create default config if it does not exist#320hiiwave wants to merge 1 commit intojazzband:masterfrom
Conversation
|
I did not notice that there's python 2.7 support (thus CI failed). Please review the concept first and I can deal with it if this is going to be merged. |
jmaupetit
left a comment
jmaupetit
left a comment
There was a problem hiding this comment.
I like the idea, thanks 👍
|
|
||
| [default_tags] | ||
| # some_project = some_tag1 some_tag2 | ||
| """).strip()) |
There was a problem hiding this comment.
🤔 Maybe we should define default values in a dedicated module (e.g. defaults.py) and use them both in the CLI and here. WDYT?
There was a problem hiding this comment.
If we had a default config, we actually don't need default value in CLI.
There was a problem hiding this comment.
I have considered an approach of adding another file default.cfg containing this multi-line string, instead of hard-coding it under watson/watson.py. But in this way we need to go through the process to include non-python files with setup.py. I'm not sure if it's what you want.
There was a problem hiding this comment.
No, I like the way you generate it, but I think it would be more pythonic (at least more explicit) to declare defaults as code and then generate a default configuration from those.
There was a problem hiding this comment.
Are you suggesting something like this?
# defaults.py
configs = {
'options': {
'confirm_new_project': False,
# ...
}
}There was a problem hiding this comment.
I agree that being explicit is always favorable, though I'm not really into this style. One drawback includes that this does not differentiate configs from different sections. And I actually think that a default config itself (e.g. defaults.cfg as follows) is more explicit than a config file generated from a module with default variable declarations. As a friendly reminder, we don't need default values in CLI anymore as we already cover them in default config.
# defaults.cfg
[options]
confirm_new_project = false
There was a problem hiding this comment.
I get you point, but still, I think using a configuration-file-driven-approach because of the CLI is not ideal. When people are using parts of Watson as a library I think it's better to avoid loading defaults from a configuration file.
There was a problem hiding this comment.
I like your point if there's future plan to support calling watson api as a library. As currently watson.watson has strong coupling with watson.config, it's not yet ready for such usage case. Though it's worth some discussion if it's on the blueprint.
IMHO, a better design will be creating a class WastonOption serving as a value object (with default values defined), and it can optionally sync with the config file. For instance, in Watson.add it will become
default_tags = self.options.default_tags instead of current default_tags = self.config.getlist('default_tags', project). Though I feel that it's out of scope of this PR.
What do you think?
| except (IOError, OSError): | ||
| rawconfig = '' | ||
| with open(watson.config_file) as fp: | ||
| rawconfig = fp.read() |
There was a problem hiding this comment.
I think that's still a nice thing to be resilient in case we can't find the config file.
3 participants
I feel that it would be helpful if there's a default config when users do
watson config -eat the first time, instead of starting from scratch.What do you think about this proposal?