----- "James Laska" <jlaska(a)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?
>
> >
http://docs.python.org/library/configparser.html
>
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