[PATCH] Re: Allow to specify remote hostname even for the Gmail case

[Nicolas Sebrecht]

On Tue, May 24, 2011 at 10:50:07AM +0200, Sebastian Spaeth wrote:
> On Fri, 20 May 2011 00:30:06 +0200, Nicolas Sebrecht <nicolas.s-dev at laposte.net> wrote:

> > It strongly break used convention all over the curernt code (for this
> > file, at least). This method renamed alone, I'm not convinced it's an
> > improvement. It may be part of another topic, though. Please, split this
> > change out of this topic!
> 
> Right, I was doing this as there was a discussion here earlier on
> whether converting those name to a more modern version would be ok, and
> you were quite indifferent.

I don't remember, sorry. Anyway, looking at the result it breaks
convention a bit too much FMPOV.

>                             Will revert this change and split in a
> different topic if needed.

Thanks.

> > > -class BaseRepository(CustomConfig.ConfigHelperMixin):
> > > +class BaseRepository(object, CustomConfig.ConfigHelperMixin):
> 
> > May I ask why this changeset? What's wrong with
> > BaseRepository(CustomConfig.ConfigHelperMixin)? Am I missing something
> > so obvious?
> 
> Yeah, we need the classes to be "new object style" classes, so we can
> later simply do:
> > > +        host = super(GmailRepository, self).get_host()
> to call our "parent" implementation of get_host()

Thanks.

> > > +        self._host = None
> > 
> > I don't understand why you decided to use self._host instead of
> > self.host like all attributes around.
> 
> Just to make clear that this is an internal implementation that is not
> supposed to be accessed from the outside (we have the acccessor function
> gethost() for that). Ideally, I would love to make it just a "host"
> @property....

It doesn't hurt, does it?

> > > +        # no success
> > > +        return None
> > 
> > I'm not sure this is the best implementation design. Returning None here
> > hurts me a bit.
> 
> Should we rather raise an Exception in this case? Which would you the
> right one?
>  
> > I would have expected gethost() to raise any valuable
> > OfflineImapError().
> 
> In the Gmail case, it could be normal and expected that the user doesn't
> configure a remote host name, so it's not always an error. I would
> rather throw and error at connection time if we still don't have a host
> name. But we can throw an OfflineImapError here too, and catch it in the
> Gmail case, fine with me.

I think it makes sense. In the usual case we need a host. Gmail the an
exception.

> Will resend a new version soon. A bit busy at work.

No problem. :-)

-- 
Nicolas Sebrecht