I'm currently writing a web framework and I wanted to add a configuration module for reading environment variables and a configuration toml file.
Here is the code. The comment in the Config
class should explain it all.
import os
import sys
import toml
BASEPATH = os.path.dirname(sys.modules['__main__'].__file__)
print(BASEPATH)
FILEPATH = os.path.abspath(os.path.join(BASEPATH, 'config.toml'))
print(FILEPATH)
class ConfigError(KeyError):
""" Raised when a requested configuration can not be found """
class ConfigBase:
def __init__(self):
self.basename = ''
self.default_options = {}
raise TypeError('Should not use ConfigBase directly')
def __getitem__(self, item):
""" called when access like a dictionary
config['database']['username']
"""
default = self.default_options.get(item)
if isinstance(default, dict):
return ConfigSection(self.basename + '.' + item, default)
# Check for env var
envvar_string = '_'.join(self.basename.split('.') + [item]).upper()
env = os.getenv(envvar_string)
if env is None:
if default is None:
raise ConfigError(
'No configuration found for {}. '
'Add the environment variable {} or add'
' the key to your config.toml file'.format(
'.'.join((self.basename, item)),
envvar_string
))
return default
return env
class Config(ConfigBase):
""" load configuration from envvar or config file
You can get settings from environment variables or a config.toml file
Environment variables have a higher precedence over the config file
Use `config['section']['subsection']['key']` to get the value
SECTION_SUBSECTION_KEY from environment variables or
section.subsection.key from a toml file
(usually written:
[section.subsection]
key = value
in a toml file)
"""
def __init__(self):
self.default_options = self.load_config()
self.basename = ''
def load_config(self):
with open(FILEPATH, 'r') as f:
config = toml.load(f)
return config
class ConfigSection(ConfigBase):
def __init__(self, basename, defaults):
self.basename = basename.lstrip('.')
self.default_options = defaults
-
\$\begingroup\$ Are you using Python 3.4 and above? \$\endgroup\$Peilonrayz– Peilonrayz ♦2017年01月10日 17:59:00 +00:00Commented Jan 10, 2017 at 17:59
-
\$\begingroup\$ Yes. 3.5+ only is fine with me \$\endgroup\$Humdinger– Humdinger2017年01月10日 18:03:21 +00:00Commented Jan 10, 2017 at 18:03
1 Answer 1
toml
is a third-party library, so should be separated from the standard library imports:
import os
import sys
import toml
ConfigBase.__init__
seems a little strange:
def __init__(self):
self.basename = ''
self.default_options = {}
raise TypeError('Should not use ConfigBase directly')
Why bother to set the attributes if you're then going to throw an error? Also you should use NotImplementedError
for abstract base classes, by convention.
Config.load_config
seems to be internal; are you expecting it to be called from anywhere other than Config.__init__
? If not, rename it _load_config
to indicate that it's an implementation detail. Also note that the temporary assignment to config
is unnecessary:
def load_config(self):
with open(FILEPATH, 'r') as f:
return toml.load(f)
I'd also note that it seems weird to add a docstring for __getitem__
that just describes what that method is for, but then not document the non-standard load_config
at all. Your target audience should already know what __getattr__
is for (or where to find out).
Talking of __getattr__
, there's an awful lot of complexity hiding in there. Every other method you've written is nice and short, but this could do with breaking down. To describe it:
- Get the default value
- If the default is a dictionary, wrap it up and return it immediately
- If not, but there's an appropriate value in the environment, return that
- If not, but the default exists, return the default
- If not, throw an error
That's how I'd structure the method:
default = self._get_default_value(item)
if isinstance(default, dict): # or the more general collections.abc.Mapping
return self._create_config_section(item, default) # or maybe return ConfigSection(self._create_basename(item), default)
env = self._get_value_from_env(item)
if env is not None:
return env
if default is not None:
return default
raise ConfigError(self._create_error_message(item))
This reads like the description I wrote above, operates at a consistent level of abstraction (not delving into the details of e.g. how to get a value from the environment or where the default comes from) and reduces the nesting.
You can split out other obvious helper methods, like self._create_env_var(item)
, along the way. Doing so might highlight to you that it's a little weird to do the same thing two different ways in the same method:
self.basename + '.' + item
'.'.join((self.basename, item))
Finally, I don't think the distinction between ConfigBase
, Config
and ConfigSection
is really helping you. You interact with them the two child classes in much the same way; only the initialisation differs. Instead, I might be inclined to do:
class Config:
def __init__(self, basename='', defaults=None):
self.basename = basename # should already be lstrip-ped
if defaults is None:
defaults = self._load_config()
self.default_options = defaults
def __getitem__(self, item):
...
...
-
\$\begingroup\$ Instead of
NotImplementedError
what about usingabc
module for abstract classes? \$\endgroup\$rdbisme– rdbisme2017年01月11日 00:51:08 +00:00Commented Jan 11, 2017 at 0:51