CFT: fix OfflineIMAP lockup in single-threaded mode

[Eygene Ryabinkin]

Wed, Jan 30, 2013 at 10:20:06AM +0100, X Ryl wrote:
> On Wed, Jan 30, 2013 at 8:42 AM, Eygene Ryabinkin <rea at codelabs.ru> wrote:
> 
> > > I'm inclined to prefer the simple "if" variant anyway, since
> > > spreading config testing everwhere is, IMHO a bad design anyway and
> > > hard to maintain.
> >
> > Could you, please, elaborate on the "bad design" and "hard to maintain"?
> >
> > When you have such code (which is mine, and I'm not proud):
> 
> key = "singlethread"  # Or worse, can come from the top class, whatever

That's perfectly grep'pable at the point where you're constructing
this key (if it is constructed from the constant string, of course).

> config.get("Repository " + repository.getname(), key)
> 
> Let's say later on, we decide to rename the config option to "nothread"
> Then if you grep the code, it's impossible to find such a pattern, so
> you'll likely introduce a bug.
> 
> Compared to:
> config.repository[repository.getname()].singlethread
> 
> Then, you can grep the code and you'll find all the places where such "key"
> is used.

Let's compare apples to apples: if someone is up to constructing the
key programmatically (by concatenation or whatever), he would use here
{{{
getattr(config.<something>, generated_key)
}}}
so your grep will be out of luck here too.

But I see the point in getting most used variants of config.get(...)
to be transformed into something more readable as 'config.repository[name]'.

Let's return to the original question, though: you're talking about
how to access the options in a better way, but I had asked why testing
configuration options in many places is a bad design and hard to
maintain.

> I consider the later code easier to maintain, obviously, and the design,
> being a bit more rigid, would impose a better code,

That's true, but only if the interface will have no need in constructing
the keys and getting them via getattr or other tricky ways.

> since you must know at write time what is the key you're reading,

Not necessarily, getattr and klass.__dict__ can be used.

> and if you do something wrong, it raises immediately (in the former
> case, you'd get empty value,

Thou won't get an empty value for nonexistent item when reading from
parsed options:
{{{
import ConfigParser

config = ConfigParser.RawConfigParser()

config.add_section('main')
config.set('main', 'time', '15')
config.set('main', 'click', 'true')

print config.get ('main', 'click')
print config.get ('main', 'none')

$ python t.py
true
Traceback (most recent call last):
  File "t.py", line 10, in <module>
    print config.get ('main', 'none')
  File "/usr/local/lib/python2.7/ConfigParser.py", line 340, in get
    raise NoOptionError(option, section)
ConfigParser.NoOptionError: No option 'none' in section: 'main'
}}}

> which will likely break, *way later*).

It is strange that you're considering a breakage here, but aren't
considering the breakage from the suggesttreads() that just returns
True to be evil.
-- 
rea