Properly re-raise exception to save original tracebacks
We usually mutate some exceptions to OfflineImapError() and it is a whole lot better if such exception will show up with the original traceback, so all valid occurrences of such mutations were transformed to the 3-tuple form of "raise". Had also added coding guidelines document where this re-raise strategy is documented. Signed-off-by: Eygene Ryabinkin <rea@codelabs.ru>
This commit is contained in:
parent
8c6abc413e
commit
e7fabf9e57
102
docs/CodingGuidelines.rst
Normal file
102
docs/CodingGuidelines.rst
Normal file
@ -0,0 +1,102 @@
|
||||
.. -*- coding: utf-8 -*-
|
||||
.. _OfflineIMAP: https://github.com/OfflineIMAP/offlineimap
|
||||
.. _OLI_git_repo: git://github.com/OfflineIMAP/offlineimap.git
|
||||
|
||||
=================================
|
||||
Coding guidelines for OfflineIMAP
|
||||
=================================
|
||||
|
||||
.. contents::
|
||||
.. .. sectnum::
|
||||
|
||||
This document contains assorted guidelines for programmers that want
|
||||
to hack OfflineIMAP.
|
||||
|
||||
|
||||
------------------
|
||||
Exception handling
|
||||
------------------
|
||||
|
||||
OfflineIMAP on many occasions re-raises various exceptions and often
|
||||
changes exception type to `OfflineImapError`. This is not a problem
|
||||
per se, but you must always remember that we need to preserve original
|
||||
tracebacks. This is not hard if you follow these simple rules.
|
||||
|
||||
For re-raising original exceptions, just use::
|
||||
|
||||
raise
|
||||
|
||||
from inside your exception handling code.
|
||||
|
||||
If you need to change exception type, or its argument, or whatever,
|
||||
use this three-argument form::
|
||||
|
||||
raise YourExceptionClass(argum, ents), None, sys.exc_info()[2]
|
||||
|
||||
In this form, you're creating an instance of new exception, so ``raise``
|
||||
will deduce its ``type`` and ``value`` parameters from the first argument,
|
||||
thus the second expression passed to ``raise`` is always ``None``.
|
||||
And the third one is the traceback object obtained from the thread-safe
|
||||
``exc_info()`` function.
|
||||
|
||||
In fact, if you hadn't already imported the whole ``sys`` module, it will
|
||||
be better to import just ``exc_info()``::
|
||||
|
||||
from sys import exc_info
|
||||
|
||||
and raise like this::
|
||||
|
||||
raise YourExceptionClass(argum, ents), None, exc_info()[2]
|
||||
|
||||
since this is the historically-preferred style in the OfflineIMAP code.
|
||||
.. -*- coding: utf-8 -*-
|
||||
.. _OfflineIMAP: https://github.com/OfflineIMAP/offlineimap
|
||||
.. _OLI_git_repo: git://github.com/OfflineIMAP/offlineimap.git
|
||||
|
||||
=================================
|
||||
Coding guidelines for OfflineIMAP
|
||||
=================================
|
||||
|
||||
.. contents::
|
||||
.. .. sectnum::
|
||||
|
||||
This document contains assorted guidelines for programmers that want
|
||||
to hack OfflineIMAP.
|
||||
|
||||
|
||||
------------------
|
||||
Exception handling
|
||||
------------------
|
||||
|
||||
OfflineIMAP on many occasions re-raises various exceptions and often
|
||||
changes exception type to `OfflineImapError`. This is not a problem
|
||||
per se, but you must always remember that we need to preserve original
|
||||
tracebacks. This is not hard if you follow these simple rules.
|
||||
|
||||
For re-raising original exceptions, just use::
|
||||
|
||||
raise
|
||||
|
||||
from inside your exception handling code.
|
||||
|
||||
If you need to change exception type, or its argument, or whatever,
|
||||
use this three-argument form::
|
||||
|
||||
raise YourExceptionClass(argum, ents), None, sys.exc_info()[2]
|
||||
|
||||
In this form, you're creating an instance of new exception, so ``raise``
|
||||
will deduce its ``type`` and ``value`` parameters from the first argument,
|
||||
thus the second expression passed to ``raise`` is always ``None``.
|
||||
And the third one is the traceback object obtained from the thread-safe
|
||||
``exc_info()`` function.
|
||||
|
||||
In fact, if you hadn't already imported the whole ``sys`` module, it will
|
||||
be better to import just ``exc_info()``::
|
||||
|
||||
from sys import exc_info
|
||||
|
||||
and raise like this::
|
||||
|
||||
raise YourExceptionClass(argum, ents), None, exc_info()[2]
|
||||
|
||||
since this is the historically-preferred style in the OfflineIMAP code.
|
1
docs/doc-src/CodingGuidelines.rst
Symbolic link
1
docs/doc-src/CodingGuidelines.rst
Symbolic link
@ -0,0 +1 @@
|
||||
../CodingGuidelines.rst
|
@ -16,6 +16,7 @@
|
||||
|
||||
import os
|
||||
import re
|
||||
from sys import exc_info
|
||||
|
||||
try:
|
||||
from ConfigParser import SafeConfigParser, Error
|
||||
@ -75,7 +76,7 @@ class CustomConfigParser(SafeConfigParser):
|
||||
return re.split(separator_re, val)
|
||||
except re.error as e:
|
||||
raise Error("Bad split regexp '%s': %s" % \
|
||||
(separator_re, e))
|
||||
(separator_re, e)), None, exc_info()[2]
|
||||
|
||||
def getdefaultlist(self, section, option, default, separator_re):
|
||||
"""Same as getlist, but returns the value of `default`
|
||||
|
@ -210,7 +210,8 @@ class SyncableAccount(Account):
|
||||
self._lockfd.close()
|
||||
raise OfflineImapError("Could not lock account %s. Is another "
|
||||
"instance using this account?" % self,
|
||||
OfflineImapError.ERROR.REPO)
|
||||
OfflineImapError.ERROR.REPO), \
|
||||
None, exc_info()[2]
|
||||
|
||||
def _unlock(self):
|
||||
"""Unlock the account, deleting the lock file"""
|
||||
|
@ -17,6 +17,7 @@
|
||||
# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
|
||||
|
||||
import re
|
||||
from sys import exc_info
|
||||
|
||||
from offlineimap import imaputil, OfflineImapError
|
||||
from offlineimap import imaplibutil
|
||||
@ -144,7 +145,8 @@ class GmailFolder(IMAPFolder):
|
||||
raise OfflineImapError("FETCHING UIDs in folder [%s]%s failed. " % \
|
||||
(self.getrepository(), self) + \
|
||||
"Server responded '[%s] %s'" % \
|
||||
(res_type, response), OfflineImapError.ERROR.FOLDER)
|
||||
(res_type, response), OfflineImapError.ERROR.FOLDER), \
|
||||
None, exc_info()[2]
|
||||
finally:
|
||||
self.imapserver.releaseconnection(imapobj)
|
||||
|
||||
|
@ -17,6 +17,7 @@
|
||||
|
||||
|
||||
import os
|
||||
from sys import exc_info
|
||||
from .Maildir import MaildirFolder
|
||||
from offlineimap import OfflineImapError
|
||||
import offlineimap.accounts
|
||||
@ -170,7 +171,8 @@ class GmailMaildirFolder(MaildirFolder):
|
||||
os.rename(tmppath, filepath)
|
||||
except OSError as e:
|
||||
raise OfflineImapError("Can't rename file '%s' to '%s': %s" % \
|
||||
(tmppath, filepath, e[1]), OfflineImapError.ERROR.FOLDER)
|
||||
(tmppath, filepath, e[1]), OfflineImapError.ERROR.FOLDER), \
|
||||
None, exc_info()[2]
|
||||
|
||||
if rtime != None:
|
||||
os.utime(filepath, (rtime, rtime))
|
||||
|
@ -594,7 +594,9 @@ class IMAPFolder(BaseFolder):
|
||||
"repository '%s' failed (abort). Server responded: %s\n"
|
||||
"Message content was: %s"%
|
||||
(msg_id, self, self.getrepository(), str(e), dbg_output),
|
||||
OfflineImapError.ERROR.MESSAGE)
|
||||
OfflineImapError.ERROR.MESSAGE), \
|
||||
None, exc_info()[2]
|
||||
# XXX: is this still needed?
|
||||
self.ui.error(e, exc_info()[2])
|
||||
except imapobj.error as e: # APPEND failed
|
||||
# If the server responds with 'BAD', append()
|
||||
@ -604,8 +606,8 @@ class IMAPFolder(BaseFolder):
|
||||
imapobj = None
|
||||
raise OfflineImapError("Saving msg (%s) folder '%s', repo '%s'"
|
||||
"failed (error). Server responded: %s\nMessage content was: "
|
||||
"%s"% (msg_id, self, self.getrepository(), str(e), dbg_output),
|
||||
OfflineImapError.ERROR.MESSAGE)
|
||||
"%s" % (msg_id, self, self.getrepository(), str(e), dbg_output),
|
||||
OfflineImapError.ERROR.MESSAGE), None, exc_info()[2]
|
||||
# Checkpoint. Let it write out stuff, etc. Eg searches for
|
||||
# just uploaded messages won't work if we don't do this.
|
||||
(typ,dat) = imapobj.check()
|
||||
@ -676,6 +678,7 @@ class IMAPFolder(BaseFolder):
|
||||
imapobj = self.imapserver.acquireconnection()
|
||||
self.ui.error(e, exc_info()[2])
|
||||
fails_left -= 1
|
||||
# self.ui.error() will show the original traceback
|
||||
if not fails_left:
|
||||
raise e
|
||||
if data == [None] or res_type != 'OK':
|
||||
|
@ -15,6 +15,7 @@
|
||||
# along with this program; if not, write to the Free Software
|
||||
# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
|
||||
|
||||
from sys import exc_info
|
||||
import os
|
||||
import threading
|
||||
|
||||
@ -79,7 +80,7 @@ class LocalStatusFolder(BaseFolder):
|
||||
errstr = "Corrupt line '%s' in cache file '%s'" % \
|
||||
(line, self.filename)
|
||||
self.ui.warn(errstr)
|
||||
raise ValueError(errstr)
|
||||
raise ValueError(errstr), None, exc_info()[2]
|
||||
self.messagelist[uid] = self.msglist_item_initializer(uid)
|
||||
self.messagelist[uid]['flags'] = flags
|
||||
|
||||
@ -103,7 +104,7 @@ class LocalStatusFolder(BaseFolder):
|
||||
errstr = "Corrupt line '%s' in cache file '%s'" % \
|
||||
(line, self.filename)
|
||||
self.ui.warn(errstr)
|
||||
raise ValueError(errstr)
|
||||
raise ValueError(errstr), None, exc_info()[2]
|
||||
self.messagelist[uid] = self.msglist_item_initializer(uid)
|
||||
self.messagelist[uid]['flags'] = flags
|
||||
self.messagelist[uid]['mtime'] = mtime
|
||||
|
@ -16,6 +16,7 @@
|
||||
# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
|
||||
import os
|
||||
import re
|
||||
from sys import exc_info
|
||||
from threading import Lock
|
||||
from .Base import BaseFolder
|
||||
try:
|
||||
@ -66,7 +67,7 @@ class LocalStatusSQLiteFolder(BaseFolder):
|
||||
except NameError:
|
||||
# sqlite import had failed
|
||||
raise UserWarning('SQLite backend chosen, but no sqlite python '
|
||||
'bindings available. Please install.')
|
||||
'bindings available. Please install.'), None, exc_info()[2]
|
||||
|
||||
#Make sure sqlite is in multithreading SERIALIZE mode
|
||||
assert sqlite.threadsafety == 1, 'Your sqlite is not multithreading safe.'
|
||||
|
@ -19,6 +19,7 @@ import socket
|
||||
import time
|
||||
import re
|
||||
import os
|
||||
from sys import exc_info
|
||||
from .Base import BaseFolder
|
||||
from threading import Lock
|
||||
try:
|
||||
@ -282,7 +283,7 @@ class MaildirFolder(BaseFolder):
|
||||
continue
|
||||
severity = OfflineImapError.ERROR.MESSAGE
|
||||
raise OfflineImapError("Unique filename %s already exists." % \
|
||||
filename, severity)
|
||||
filename, severity), None, exc_info()[2]
|
||||
else:
|
||||
raise
|
||||
|
||||
@ -372,7 +373,8 @@ class MaildirFolder(BaseFolder):
|
||||
except OSError as e:
|
||||
raise OfflineImapError("Can't rename file '%s' to '%s': %s" % (
|
||||
oldfilename, newfilename, e[1]),
|
||||
OfflineImapError.ERROR.FOLDER)
|
||||
OfflineImapError.ERROR.FOLDER), \
|
||||
None, exc_info()[2]
|
||||
|
||||
self.messagelist[uid]['flags'] = flags
|
||||
self.messagelist[uid]['filename'] = newfilename
|
||||
|
@ -14,6 +14,8 @@
|
||||
# You should have received a copy of the GNU General Public License
|
||||
# along with this program; if not, write to the Free Software
|
||||
# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
|
||||
|
||||
from sys import exc_info
|
||||
from threading import Lock
|
||||
from offlineimap import OfflineImapError
|
||||
from .IMAP import IMAPFolder
|
||||
@ -60,7 +62,7 @@ class MappedIMAPFolder(IMAPFolder):
|
||||
line = line.strip()
|
||||
except ValueError:
|
||||
raise Exception("Corrupt line '%s' in UID mapping file '%s'"%
|
||||
(line, mapfilename))
|
||||
(line, mapfilename)), None, exc_info()[2]
|
||||
(str1, str2) = line.split(':')
|
||||
loc = long(str1)
|
||||
rem = long(str2)
|
||||
@ -89,7 +91,7 @@ class MappedIMAPFolder(IMAPFolder):
|
||||
raise OfflineImapError("Could not find UID for msg '{0}' (f:'{1}'."
|
||||
" This is usually a bad thing and should be reported on the ma"
|
||||
"iling list.".format(e.args[0], self),
|
||||
OfflineImapError.ERROR.MESSAGE)
|
||||
OfflineImapError.ERROR.MESSAGE), None, exc_info()[2]
|
||||
|
||||
# Interface from BaseFolder
|
||||
def cachemessagelist(self):
|
||||
|
@ -18,6 +18,7 @@ import os
|
||||
import fcntl
|
||||
import time
|
||||
import subprocess
|
||||
from sys import exc_info
|
||||
import threading
|
||||
from hashlib import sha1
|
||||
|
||||
@ -54,7 +55,7 @@ class UsefulIMAPMixIn(object):
|
||||
errstr = "Server '%s' closed connection, error on SELECT '%s'. Ser"\
|
||||
"ver said: %s" % (self.host, mailbox, e.args[0])
|
||||
severity = OfflineImapError.ERROR.FOLDER_RETRY
|
||||
raise OfflineImapError(errstr, severity)
|
||||
raise OfflineImapError(errstr, severity), None, exc_info()[2]
|
||||
if result[0] != 'OK':
|
||||
#in case of error, bail out with OfflineImapError
|
||||
errstr = "Error SELECTing mailbox '%s', server reply:\n%s" %\
|
||||
|
@ -206,8 +206,8 @@ class IMAPServer:
|
||||
imapobj.starttls()
|
||||
except imapobj.error as e:
|
||||
raise OfflineImapError("Failed to start "
|
||||
"TLS connection: %s"% str(e),
|
||||
OfflineImapError.ERROR.REPO)
|
||||
"TLS connection: %s" % str(e),
|
||||
OfflineImapError.ERROR.REPO, None, exc_info()[2])
|
||||
|
||||
|
||||
## All __authn_* procedures are helpers that do authentication.
|
||||
@ -466,7 +466,7 @@ class IMAPServer:
|
||||
"'%s'. Make sure you have configured the ser"\
|
||||
"ver name correctly and that you are online."%\
|
||||
(self.hostname, self.repos)
|
||||
raise OfflineImapError(reason, severity)
|
||||
raise OfflineImapError(reason, severity), None, exc_info()[2]
|
||||
|
||||
elif isinstance(e, SSLError) and e.errno == 1:
|
||||
# SSL unknown protocol error
|
||||
@ -479,7 +479,7 @@ class IMAPServer:
|
||||
reason = "Unknown SSL protocol connecting to host '%s' for "\
|
||||
"repository '%s'. OpenSSL responded:\n%s"\
|
||||
% (self.hostname, self.repos, e)
|
||||
raise OfflineImapError(reason, severity)
|
||||
raise OfflineImapError(reason, severity), None, exc_info()[2]
|
||||
|
||||
elif isinstance(e, socket.error) and e.args[0] == errno.ECONNREFUSED:
|
||||
# "Connection refused", can be a non-existing port, or an unauthorized
|
||||
@ -487,15 +487,15 @@ class IMAPServer:
|
||||
reason = "Connection to host '%s:%d' for repository '%s' was "\
|
||||
"refused. Make sure you have the right host and port "\
|
||||
"configured and that you are actually able to access the "\
|
||||
"network."% (self.hostname, self.port, self.repos)
|
||||
raise OfflineImapError(reason, severity)
|
||||
"network." % (self.hostname, self.port, self.repos)
|
||||
raise OfflineImapError(reason, severity), None, exc_info()[2]
|
||||
# Could not acquire connection to the remote;
|
||||
# socket.error(last_error) raised
|
||||
if str(e)[:24] == "can't open socket; error":
|
||||
raise OfflineImapError("Could not connect to remote server '%s' "\
|
||||
"for repository '%s'. Remote does not answer."
|
||||
% (self.hostname, self.repos),
|
||||
OfflineImapError.ERROR.REPO)
|
||||
OfflineImapError.ERROR.REPO), None, exc_info()[2]
|
||||
else:
|
||||
# re-raise all other errors
|
||||
raise
|
||||
@ -702,7 +702,7 @@ class IdleThread(object):
|
||||
self.ui.error(e, exc_info()[2])
|
||||
self.parent.releaseconnection(imapobj, True)
|
||||
else:
|
||||
raise e
|
||||
raise
|
||||
else:
|
||||
success = True
|
||||
if "IDLE" in imapobj.capabilities:
|
||||
|
@ -103,7 +103,8 @@ class IMAPRepository(BaseRepository):
|
||||
except Exception as e:
|
||||
raise OfflineImapError("remotehosteval option for repository "\
|
||||
"'%s' failed:\n%s" % (self, e),
|
||||
OfflineImapError.ERROR.REPO)
|
||||
OfflineImapError.ERROR.REPO), \
|
||||
None, exc_info()[2]
|
||||
if host:
|
||||
self._host = host
|
||||
return self._host
|
||||
|
@ -15,6 +15,8 @@
|
||||
# along with this program; if not, write to the Free Software
|
||||
# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
|
||||
|
||||
from sys import exc_info
|
||||
|
||||
try:
|
||||
from configparser import NoSectionError
|
||||
except ImportError: #python2
|
||||
@ -66,14 +68,16 @@ class Repository(object):
|
||||
except NoSectionError as e:
|
||||
errstr = ("Could not find section '%s' in configuration. Required "
|
||||
"for account '%s'." % ('Repository %s' % name, account))
|
||||
raise OfflineImapError(errstr, OfflineImapError.ERROR.REPO)
|
||||
raise OfflineImapError(errstr, OfflineImapError.ERROR.REPO), \
|
||||
None, exc_info()[2]
|
||||
|
||||
try:
|
||||
repo = typemap[repostype]
|
||||
except KeyError:
|
||||
errstr = "'%s' repository not supported for '%s' repositories." \
|
||||
% (repostype, reqtype)
|
||||
raise OfflineImapError(errstr, OfflineImapError.ERROR.REPO)
|
||||
raise OfflineImapError(errstr, OfflineImapError.ERROR.REPO), \
|
||||
None, exc_info()[2]
|
||||
|
||||
return repo(name, account)
|
||||
|
||||
|
Loading…
Reference in New Issue
Block a user