On Mon, 2010-12-13 at 11:01 -0500, Kamil Paral wrote:
----- "James Laska" jlaska@redhat.com wrote:
+def getbool(string):
- '''Converts a string into a boolean.
Recognizes many usual strings: 1/0, yes/no, true/false,
on/off; also
empty strings.
Throws ValueError if the string can't be recognized.
- '''
- boolean_states = {'1': True, 'yes': True, 'true': True,
'on':
True,
'0': False, 'no': False, 'false': False,
'off':
False}
if not string:
return False
string = string.lower()
if string not in boolean_states:
raise ValueError, 'Not a boolean: %s' % string
return boolean_states[string]
Everything looks good. Only one comment regarding the getbool() method. Doesn't SafeConfigParser provide different methods to return a value of a specific type? For example, getboolean(), getfloat(), getint(). Should we be using (or overloading in the case of getboolean) those built-in methods instead?
Great comment. If you look at ConfigParser source code, the implementation is almost identical:
_boolean_states = {'1': True, 'yes': True, 'true': True, 'on': True, '0': False, 'no': False, 'false': False, 'off': False} def getboolean(self, section, option): v = self.get(section, option) if v.lower() not in self._boolean_states: raise ValueError, 'Not a boolean: %s' % v return self._boolean_states[v.lower()]
Unfortunately we can't use their methods directly, because it requires section names and option names and we sometimes need just the conversion. We also should use the private attribute directly. So I basically copied that.
Of course, that creates the question - why do we in fact need to convert our config options? Why don't we extract them with correct type right away? And the answer is ticket #255 :) https://fedorahosted.org/autoqa/ticket/255
In short, we shouldn't be using config files directly from our libraries. We should have a Python class for it. getbool() is just a temporary solution (hey, what's *not* a temporary solution?:o)) for making coding less error-prone. It mainly helps with parsing the options obtained from config_loader(autoqa_conf), which is not an ConfigParser object at all (this should be changed, too).
Did I manage to explain it, or obscure it?
Spot on, thanks Kamil. I agree, I think it'll be fewer headaches for us the *less* we diverge from ConfigParser. I like the idea of using autoqa.config (ticket#255) to define a single interface to autoqa.conf. Additionally, yeah ... let's use the built-in type support where possible for reading config values.
Thanks, James