Re: [PATCH] Added a support for sending comments into Bodhi, part 2

[James Laska]

On Mon, 2010-12-13 at 11:01 -0500, Kamil Paral wrote:
>  ----- "James Laska" <jlaska(a)redhat.com&gt; 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