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

Create default config if it does not exist #320

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Create default config if it does not exist #320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions watson/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1341,11 +1341,8 @@ def config(context, key, value, edit):
wconfig = watson.config

if edit:
try:
with open(watson.config_file) as fp:
rawconfig = fp.read()
except (IOError, OSError):
rawconfig = ''
with open(watson.config_file) as fp:
rawconfig = fp.read()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's still a nice thing to be resilient in case we can't find the config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice suggestion.


newconfig = click.edit(text=rawconfig, extension='.ini')

Expand Down
27 changes: 27 additions & 0 deletions watson/watson.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import operator
import os
import uuid
from pathlib import Path
import textwrap

try:
import configparser
Expand Down Expand Up @@ -115,6 +117,7 @@ def config(self):
Return Watson's config as a ConfigParser object.
"""
if not self._config:
self._check_default_config()
try:
config = ConfigParser()
config.read(self.config_file)
Expand All @@ -134,6 +137,30 @@ def config(self, value):
self._config = value
self._config_changed = True

def _check_default_config(self):
if Path(self.config_file).exists():
return
Path(self.config_file).write_text(textwrap.dedent("""
hiiwave marked this conversation as resolved.
Show resolved Hide resolved
[backend]
# url =
# token = yourapitoken

[options]
confirm_new_project = false
confirm_new_tag = false
date_format = %Y.%m.%d
log_current = false
pager = true
report_current = false
stop_on_start = false
stop_on_restart = false
time_format = %H:%M:%S%z
week_start = monday

[default_tags]
# some_project = some_tag1 some_tag2
""").strip())
Copy link
Contributor

@jmaupetit jmaupetit Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 Maybe we should define default values in a dedicated module (e.g. defaults.py) and use them both in the CLI and here. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we had a default config, we actually don't need default value in CLI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting something like this?

# defaults.py
configs = {
    'options': {
        'confirm_new_project': False,
        # ...
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@hiiwave hiiwave Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, yay!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is out of scope of this PR, let's track this in #327. Perhaps we can set aside this issue for now, and reconsider it if #327 or similar designs are implemented someday.


def save(self):
"""
Save the state in the appropriate files. Create them if necessary.
Expand Down