Add proper config module

[Holzhaus]

Rationale

Jasper really needs a proper config module.

I saw multiple errors caused by modules/classes/functions trying to access the yaml-configuration looking for values that were not set (e.g. gmail_address). This means that we'll end up writing checking functions all over the place or using a general try: ... except: ...-block wrapped around Conversation.handleForever() (which currently prevents us from killing Jasper with CRTL-C by the way).

So the obvious solution is to create a new module (named jasperconf in this case), that does this kind of stuff for us. It offers us a config singleton, which relieves us from the obligation to pass around the profile-variable or open the config file every time we need a value.

It also adds proper separation between main and plugin namespace in the config file (all plugins access the config via plugin_get/plugin_set methods, which retrieves and sets value in plugins → plugin_name → x.
If that namespace should ever be changed - no problem, just change the jasperconf module and probably add a conversion for old config files.

Last (and probably least), it also decouples the access to config values from the actual backend (YAML) and thus makes a transition - if neccessary - (e.g. to ConfigParser) a lot easier.

Usage

Main program

from jasperconf import Config as conf
firstname = conf.get("firstname")
# Alternative:
# firstname = conf.get(["firstname"])
conf.set(["keys","GOOGLE_SPEECH"], value="ABCDEF" )
google_api_key = conf.get(["keys", "GOOGLE_SPEECH"])

Plugins (in client/modules)

PLUGIN_NAME = "foo"

from ..jasperconf import Config as conf
bar = conf.plugin_get(PLUGIN_NAME, "bar")
# Alternative:
# bar = conf.plugin_get(PLUGIN_NAME, ["bar"])
conf.plugin_set(PLUGIN_NAME, ['example','baz'], value="Doh!")
doh = conf.plugin_get(PLUGIN_NAME, ['example','baz'])

[Holzhaus]

Just wanted to add that I rewrote 'populate.py' to use the new module and tidied it up while I was at it.
It now also support output to a different file (specified via command line argument --file) and editing an config profile file. To start fresh, delete the existing config file or use --new.

It did not change any other modules because I'm not sure this get's accepted because of the ConfigParser pull request, in which you stated that you did not want to break compatability with third party plugins (if I recall that correctly). Although this is the case here, too, I hope you see the advantages of this approach.

So if you're willing to accept this PR, I will update all other modules to use jasperconf.

[Holzhaus]

So....What do you think?

[charliermarsh]

I like the idea of moving away from passing profile everywhere... Do you have an example of a module that currently needs the plugin_get/plugin_set functionality?

[Holzhaus]

Well, any plugin module that needs its own config value (Gmail.py for example).
The idea is to stop pollution of the central namespace, because every module has it's own. This would of course be archievable by the normal get/set methods, too, but this adds an addition layer of abstraction for plugins.
The main advantages are:

• It's easier for the plugin developers (no need to think about the absolute namespace path, just input the plugin name and go)
• we could move the "plugins" namespace later on (to another location inside the config file, to a separate plugins.conf file or even to separate files/folders for every plugin) without changing the plugin modules at all
• Perhaps we could add code to restrict plugins from chaning the main namespace at all (actually I don't know if that's possible...probably via the traceback module or something like that)

[charliermarsh]

Yeah, that makes sense. (Although in the Gmail.py example: you actually do need the Gmail address elsewhere in some cases (e.g., if you want Jasper to email you a list of links to news articles); but in general, the principle of polluting the global namespace does need to end.)

I'm fine with merging this in after code review and testing, but the docs would need some major updates. (At this point, it also makes sense to start discussing some sort of Jasper v2 (CC @shbhrsaha). We can continue discussion offline.)

[charliermarsh]

Can you remove these indentations in the docstrings? Don't think we do that elsewhere.

[charliermarsh]

Pending these changes, I don't think we're too far off here (although the modules need to be modified, of course).

[alexsiri7]

The logic to create the plugin path is the same in plugin_get and plugin_set, can this be abstracted?

[Holzhaus]

I guess.
I'm not completely sure if they'll useful anyway (probably they should be left out and then made methods of the plugin base class (config_set() and config_get).

[alexsiri7]

I think the plugin's config_get should call the config's plugin_get, same thing for set.

[Holzhaus]

They could just call config.set(['plugins', self.SLUG, 'foo'], value='bar')?

[alexsiri7]

Yes... but I like the idea of just saying config_set('foo', 'bar'). Simple, less duplication, less errors.

[Holzhaus]

Yes, my comment referred to the inner workings of a method that belongs to the base plugin class (i.e. the class that all Plugins inherit from). Therefore, every Plugin will be able to do self.config_set(foo, bar) by default.

[Holzhaus]

I'd like to suggest that I remove the plugin_set/plugin_get method and add a to_dict() method for now, so that we can merge this PR before we reach v2.0.

This way, we can simplify the core jasper code and use it right now. Modules get the dict generated by the to_dict() method as profile argument (which will look just like the dict generated by yaml.safe_load()), so there'll be no changes neccessary in client modules.

[Holzhaus]

What do you think?

[alexsiri7]

I think it's a good idea to allow v1 modules to be compatible. This sounds quite reasonable. We should also check whether the method requires the parameter, and if not then not send it (that will allow modules to stop receiving the parameter and start using the new interface).

[Holzhaus]

I removed plugin_get/plugin_set, added the to_dict() method, added logging and rebased to the latest master version.

[Holzhaus]

Going to start integration after merge of #199, #176 and #155 to avoid ending up with a lot of merge conflicts.

[coveralls]

Coverage remained the same when pulling 9ff4d39 on Holzhaus:proper-config into 3ad1cee on jasperproject:master.

[coveralls]

Coverage remained the same when pulling 3245550 on Holzhaus:proper-config into 56f65ff on jasperproject:master.