Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I've never done anything like this before so I'd like someone else to look at this before I get too carried away :)

Am I making this more complicated than it needs to be? I'm trying to make it EASY for other modules/scripts on this system to store and retrieve their settings. Hence why I trap the ConfigParser.NoOptionError error and return None and create the section if it doesn't exist in the set() method.

Suggestions?

import ConfigParser
import os

from ast import literal_eval as Eval

class _ConfParse(ConfigParser.ConfigParser):
    def __init__(self, confpath, conffile):
        ConfigParser.ConfigParser.__init__(self)
        self.conf_file = os.path.join(confpath, conffile)
        try: self.readfp(open(self.conf_file), 'r')
        except IOError as Err:
            if Err.errno == 2: pass
            else: raise Err

    def set(self, section, option, value):
        if self.has_section(section):
            ConfigParser.ConfigParser.set(self, section, option, str(value))
        else:
            self.add_section(section)
            ConfigParser.ConfigParser.set(self, section, option, str(value))

    def get(self, section, option):
        try: return Eval(ConfigParser.ConfigParser.get(self, section, option))
        except ConfigParser.NoOptionError: return None

    def save(self):
        self.write(open(self.conf_file, 'w'))

    def __del__(self):
        self.save()

class LocalConfig(_ConfParse):
    def __init__(self, conffile, confpath = '/etc/local/cnf'):
        _ConfParse.__init__(self, confpath, conffile)

class SysConfig(_ConfParse):
    def __init__(self, conffile, confpath = '/etc/sys/cnf'):
        _ConfParse.__init__(self, confpath, conffile)
share|improve this question

1 Answer

up vote 1 down vote accepted

One problem I see is that returning None from your modified get() method conflicts with the normal case of a valueless option (from the bottom of the module docs for ConfigParser):

>>> import ConfigParser
>>> import io

>>> sample_config = """
... [mysqld]
... user = mysql
... pid-file = /var/run/mysqld/mysqld.pid
... skip-external-locking
... old_passwords = 1
... skip-bdb
... skip-innodb
... """
>>> config = ConfigParser.RawConfigParser(allow_no_value=True)
>>> config.readfp(io.BytesIO(sample_config))

>>> # Settings with values are treated as before:
>>> config.get("mysqld", "user")
'mysql'

>>> # Settings without values provide None:
>>> config.get("mysqld", "skip-bdb")

>>> # Settings which aren't specified still raise an error:
>>> config.get("mysqld", "does-not-exist")
Traceback (most recent call last):
  ...
ConfigParser.NoOptionError: No option 'does-not-exist' in section: 'mysqld'

Note the second-to-last example commented as "Settings without values provide None:" Of course this isn't an issue if you intend to exclude this sort of option. Other than that, I like the auto-section feature.

Though, I'd lean towards adding to the interface rather than masking and changing the behavior, so instead of replacing get/set, add safe_ versions:

def safe_set(self, section, option, value):
    if self.has_section(section):
        self.set(section, option, str(value))
    else:
        self.add_section(section)
        self.set(section, option, str(value))

def safe_get(self, section, option):
    if self.has_option(section, option):
        return self.get(section, option)
    else:
        return None

This would make it more flexible as code would still have access to ConfigParser's normal interface and the option of using the "safe" calls which don't throw exceptions.

share|improve this answer
I thought about creating a different 'safe' method, but since I'm not doing anything to mess with the format of the file; you could still use the standard ConfigParser on the same file. Is retaining the inherited class methods un-modified a 'best' or 'standard' practice? I just don't know what the conventions are for something like this- if there are any! Thanks for looking at this! – tMC Jun 3 '11 at 20:51

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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