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

Create default config if it does not exist #320

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

Conversation

hiiwave
Copy link
Contributor

@hiiwave hiiwave commented Sep 20, 2019

I feel that it would be helpful if there's a default config when users do watson config -e at the first time, instead of starting from scratch.

What do you think about this proposal?

@hiiwave
Copy link
Contributor Author

hiiwave commented Sep 21, 2019

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.

@hiiwave hiiwave mentioned this pull request Sep 22, 2019
Copy link
Contributor

@jmaupetit jmaupetit left a comment

Choose a reason for hiding this comment

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

I like the idea, thanks 👍

watson/watson.py Show resolved Hide resolved

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

@jmaupetit jmaupetit Sep 30, 2019

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

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.

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.

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.

3 participants