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
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Jan 10, 2017 at 17:52
\$\endgroup\$
2
  • \$\begingroup\$ Are you using Python 3.4 and above? \$\endgroup\$ Commented Jan 10, 2017 at 17:59
  • \$\begingroup\$ Yes. 3.5+ only is fine with me \$\endgroup\$ Commented Jan 10, 2017 at 18:03

1 Answer 1

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):
 ...
 ...
answered Jan 10, 2017 at 23:16
\$\endgroup\$
1
  • \$\begingroup\$ Instead of NotImplementedError what about using abc module for abstract classes? \$\endgroup\$ Commented Jan 11, 2017 at 0:51

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.