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

[Sebastian Spaeth]

On Fri, 20 May 2011 00:30:06 +0200, Nicolas Sebrecht <nicolas.s-dev at laposte.net> wrote:
> It looks wrong from the original developer POV
> (offlineimap/repository/Gmail.py line 24): 

Well, I guess this has been coded when there was only imap.gmail.com,
but by now with google apps, it uses much more domain names which was
not the case back then. So changing this to

> > Rename the gethost() function to the more modern pythonic get_host()
> 
> 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. Will revert this change and split in a
different topic if needed.

> > -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()

> > +    def get_host(self):
> > +        """Return the server name to connect to.
> > +
> > +        Gmail implementation first checks for the usual IMAP settings
> > +        and falls back to imap.gmail.com if not specified."""
> 
> Which make the comment of this class lying (see above). :-/

Right that comment needs changing too...

> > +        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....
> > +                raise OfflineImapError("remotehosteval option for repository "\
> > +                                       "'%s' failed:\n%s" % (self, e),
> > +                                       OfflineImapError.ERROR.REPO)
> 
> Good! :-)

:-)

> > +        # 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.

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

Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://alioth-lists.debian.net/pipermail/offlineimap-project/attachments/20110524/c16d57b8/attachment-0001.sig>