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

[Nicolas Sebrecht]

On Thu, May 19, 2011 at 09:56:40PM +0200, Sebastian Spaeth wrote:
> 
> Previously we hard-coded the imap server name in the case of Gmail
> repositories, but often we need a different host name. So, allow people
> to specify the hostname via the regular "remotehosteval" and
> "remotehost" settings, and only falling back to imap.gmail.com when
> nothing has been specified.

It looks wrong from the original developer POV
(offlineimap/repository/Gmail.py line 24): 

"
  Uses hard-coded host name and port, see:
    http://mail.google.com/support/bin/answer.py?answer=78799&topic=12814 
"

I see he is now wrong for some _rare_ use cases but you should tell
_why_ this is a good change.

> 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!

>                                                                      and
> cache the hostname, so we don't evaluate the whole thing each time we
> query the host name.
>
> Make the remotehosteval processing more robust, by catching any
> Exceptions that occur, and throw a OfflineImapError, that explains where
> exactly the error had occured. You can test this, e.g. by setting
> remotehosteval to 1/"n" or some other invalid expression.
> 
> The whole IMAP.get_host() function has been documented code wise while
> going through.

Fine.

<...>

> diff --git a/offlineimap/repository/Base.py b/offlineimap/repository/Base.py
> index 594f864..34f2f4a 100644
> --- a/offlineimap/repository/Base.py
> +++ b/offlineimap/repository/Base.py
> @@ -21,7 +21,7 @@ import traceback
>  from offlineimap import CustomConfig
>  from offlineimap.ui import getglobalui
>  
> -class BaseRepository(CustomConfig.ConfigHelperMixin):
> +class BaseRepository(object, CustomConfig.ConfigHelperMixin):
>      def __init__(self, reposname, account):
>          self.ui = getglobalui()
>          self.account = account

May I ask why this changeset? What's wrong with
BaseRepository(CustomConfig.ConfigHelperMixin)? Am I missing something
so obvious?

> diff --git a/offlineimap/repository/Gmail.py b/offlineimap/repository/Gmail.py
> index 97637b8..2f2a73e 100644
> --- a/offlineimap/repository/Gmail.py
> +++ b/offlineimap/repository/Gmail.py
> @@ -33,16 +33,21 @@ class GmailRepository(IMAPRepository):
>      
>      def __init__(self, reposname, account):
>          """Initialize a GmailRepository object."""
> -        account.getconfig().set('Repository ' + reposname,
> -                                'remotehost', GmailRepository.HOSTNAME)
> -        account.getconfig().set('Repository ' + reposname,
> -                                'remoteport', GmailRepository.PORT)
> +        # Enforce SSL usage
>          account.getconfig().set('Repository ' + reposname,
>                                  'ssl', 'yes')
>          IMAPRepository.__init__(self, reposname, account)
>  
> -    def gethost(self):
> -        return GmailRepository.HOSTNAME
> +    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). :-/

> +        host = super(GmailRepository, self).get_host()
> +        if host:
> +            return host
> +        self._host = GmailRepository.HOSTNAME
> +        return self._host
>  
>      def getport(self):
>          return GmailRepository.PORT
> diff --git a/offlineimap/repository/IMAP.py b/offlineimap/repository/IMAP.py
> index 0bc84eb..d83e484 100644
> --- a/offlineimap/repository/IMAP.py
> +++ b/offlineimap/repository/IMAP.py
> @@ -17,7 +17,7 @@
>  #    Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
>  
>  from offlineimap.repository.Base import BaseRepository
> -from offlineimap import folder, imaputil, imapserver
> +from offlineimap import folder, imaputil, imapserver, OfflineImapError
>  from offlineimap.folder.UIDMaps import MappedIMAPFolder
>  from offlineimap.threadutil import ExitNotifyThread
>  from threading import Event
> @@ -32,6 +32,7 @@ class IMAPRepository(BaseRepository):
>          """Initialize an IMAPRepository object."""
>          BaseRepository.__init__(self, reposname, account)
>          # self.ui is being set by the BaseRepository
> +        self._host = None

I don't understand why you decided to use self._host instead of
self.host like all attributes around.

>          self.imapserver = imapserver.ConfigedIMAPServer(self)
>          self.folders = None
>          self.nametrans = lambda foldername: foldername
> @@ -87,18 +88,32 @@ class IMAPRepository(BaseRepository):
>      def getsep(self):
>          return self.imapserver.delim
>  
> -    def gethost(self):
> -        host = None
> -        localeval = self.localeval
> +    def get_host(self):
> +        """Return the hostname to connect to
> +
> +        :returns: hostname as string or None if none configured"""
> +        if self._host:  # use cached value if possible
> +            return self._host
>  
> +        # 1) check for remotehosteval setting
>          if self.config.has_option(self.getsection(), 'remotehosteval'):
>              host = self.getconf('remotehosteval')
> +            try:
> +                host = self.localeval.eval(host)
> +            except Exception, e:
> +                raise OfflineImapError("remotehosteval option for repository "\
> +                                       "'%s' failed:\n%s" % (self, e),
> +                                       OfflineImapError.ERROR.REPO)

Good! :-)

> +            if host:
> +                self._host = host
> +                return self._host
> +        # 2) check for plain remotehost setting
> +        host = self.getconf('remotehost', None)
>          if host != None:
> -            return localeval.eval(host)
> -
> -        host = self.getconf('remotehost')
> -        if host != None:
> -            return host
> +            self._host = host
> +            return self._host
> +        # no success
> +        return None

I'm not sure this is the best implementation design. Returning None here
hurts me a bit.

I would have expected gethost() to raise any valuable
OfflineImapError().

If some inherited class can redefine the host user configuration by
itself, we may need to factorise out the process "get the user host
definition" in a dedicated method of IMAPRepository().  Then, the same
code could be called from gethost() and Gmail.

>      def getuser(self):
>          user = None

-- 
Nicolas Sebrecht