Add config loader from config file #146

eSoares · 2017-11-15T23:18:55Z

With this config we load configuration variables from a config file and from the environment.
If a variable is present in both (config and env) the env variable is used.
This makes easy to quick change a particular variable between restarts (the debug level e.g.).

…iable With this config we load configuration variables from a config file and from the environment. If a variable is in both (config and env) the env variable is used. This makes easy to quick change a particular variable between restarts (the debug level e.g.).

eSoares · 2017-11-16T00:34:51Z

Just to add to the idea of loading tokens/vars from a config file.
Each plugin could have it's own section with their parameters, something like:

.....
[plugin_weather]
token = ....
default_city = ...

llimllib

I left detailed notes on the implementation, but I think I'm still not sold on the why to add this. Can you make the case for having a config file again?

In my mind, you could always just write a tiny bash script that, on startup, dumped config variables from a file into the environment. Why would this be better than that?

llimllib · 2017-11-18T03:09:54Z

limbo/config.py

@@ -0,0 +1,50 @@
+import os
+from backports import configparser as confparser


Where does backports come from?

Gives some backports of features from newer versions of Python to previews: https://pypi.python.org/pypi/configparser

llimllib · 2017-11-18T03:10:28Z

limbo/config.py

+from backports import configparser as confparser
+
+
+class Config:


should be class Config(object)

llimllib · 2017-11-18T03:15:04Z

limbo/config.py

+
+
+class Config:
+    env_config = {}


This should be an instance variable, not a class-level variable. Later on, when you say self.env_config, you're not referring to this variable but creating a new instance variable; this one is available as Config.env_config but is never used

llimllib · 2017-11-18T03:15:11Z

limbo/config.py

+
+class Config:
+    env_config = {}
+    file_config = None  # type: confparser.ConfigParser


llimllib · 2017-11-18T03:15:35Z

limbo/config.py

+        self.file_config = confparser.ConfigParser()
+        config_file = self.env_config.get("config_location", "config.ini")
+        # check if config exists
+        import os


import this at the top of the file, not in here

llimllib · 2017-11-18T03:19:16Z

limbo/config.py

+        # check if config exists
+        import os
+        if not os.path.exists(config_file):
+            raise IOError("File for configuration not found")


Could we just log it and move on?

Yes, it can be.

llimllib · 2017-11-18T03:20:52Z

limbo/config.py

+
+    def load_config_file(self):
+        self.file_config = confparser.ConfigParser()
+        config_file = self.env_config.get("config_location", "config.ini")


Does this really need to be configurable? Couldn't we just say that the config file is named config.ini?

It makes testing easier, to change between mock files. Also adds easier configuration.

llimllib · 2017-11-18T03:23:21Z

limbo/config.py

+        self.load_config_file()
+
+    def load_config_file(self):
+        self.file_config = confparser.ConfigParser()


Why store file config separately from environment config? If we allow that we should accept both, shouldn't they go into the same object?

In the future we can easy add option to extensions to write to config file, not just read.
For that we should keep the ConfigParser object.

llimllib · 2017-11-18T03:24:25Z

requirements.txt

@@ -35,3 +35,4 @@ virtualenv==15.1.0
 webencodings==0.5.1
 websocket-client==0.44.0
 wrapt==1.10.10
+configparser==3.5.0


why add a third-party lib when python has this available in all the versions we use?

Indeed, was my mistake. The IDE kept saying it wasn't installed and I thought it was needed for Python 2.
Will remove in future commit then.

# Conflicts: # limbo/limbo.py

eSoares added 3 commits November 15, 2017 23:14

Add missing file from last commit with the config definition

8777370

Fix config calling in some cases

9b5669b

llimllib reviewed Nov 18, 2017

View reviewed changes

eSoares added 3 commits November 18, 2017 17:58

Requested fixes and changes

58c5140

Fix to support non existence of config file

2f24390

Wrong order of operations making clean the config file after loading it

8789708

eSoares mentioned this pull request Jan 24, 2018

Update use of Docker #153

Closed

eSoares added 2 commits September 23, 2018 22:46

Merge branch 'master' into feature/config_file_support

1bfae95

# Conflicts: # limbo/limbo.py

Fix importing backports that is missing in python3

ddb4b32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config loader from config file #146

Add config loader from config file #146

eSoares commented Nov 15, 2017

eSoares commented Nov 16, 2017

llimllib left a comment

llimllib Nov 18, 2017

eSoares Nov 18, 2017

llimllib Nov 18, 2017

llimllib Nov 18, 2017

llimllib Nov 18, 2017

llimllib Nov 18, 2017

eSoares Nov 18, 2017

llimllib Nov 18, 2017

eSoares Nov 18, 2017

llimllib Nov 18, 2017

eSoares Nov 18, 2017

llimllib Nov 18, 2017

eSoares Nov 18, 2017

llimllib Nov 18, 2017

eSoares Nov 18, 2017

		@@ -0,0 +1,50 @@
		import os
		from backports import configparser as confparser

		from backports import configparser as confparser


		class Config:

Add config loader from config file #146

Are you sure you want to change the base?

Add config loader from config file #146

Conversation

eSoares commented Nov 15, 2017

eSoares commented Nov 16, 2017

llimllib left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment