3
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ Are you using Python 3.4 and above? \$\endgroup\$ – Peilonrayz Jan 10 '17 at 17:59
  • \$\begingroup\$ Yes. 3.5+ only is fine with me \$\endgroup\$ – Humdinger Jan 10 '17 at 18:03
3
\$\begingroup\$

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):
        ...

    ...
\$\endgroup\$
  • \$\begingroup\$ Instead of NotImplementedError what about using abc module for abstract classes? \$\endgroup\$ – DiTTiD Jan 11 '17 at 0:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.