zope.sendmail grantma-retryfixes branch review
I'm starting a new thread, because the original one got hijacked by the doctest/unittest flamefest. I appear to have unsubscribed from zope3-checkins a while ago, which is why I'm posting here. Disclaimer: I'm not very good at doing code reviews. I tend to mention every little detail that could've been improved and forget to praise everything that's already perfect. There are two checkins on the branch. ========= rev 84645 ========= http://svn.zope.org/zope.sendmail/branches/grantma-retryfixes/?rev=84645&vie... Matthew, Subversion thinks the two PDF files you added are plain-text (svn:eol-style is "native", and svn diff shows the actual contents of the PDFs, which isn't very useful to human readers). I would suggest svn propdel svn:eol-style *.pdf svn propset svn:mime-type application/pdf *.pdf Also, the Quue_*.pdf seems to have a typo in the file name. At first sight those diagrams look scary and complicated. I'd look into ways to express their content in a text file with plain-text explanations and ASCII-art diagrams. ========= rev 84551 ========= http://svn.zope.org/zope.sendmail/branches/grantma-retryfixes/?rev=84551&vie...
Index: src/zope/sendmail/configure.zcml =================================================================== --- src/zope/sendmail/configure.zcml (revision 84550) +++ src/zope/sendmail/configure.zcml (revision 84551) @@ -14,11 +14,18 @@
<!-- To send mail, uncomment the following directive and be sure to - create the queue directory. + create the queue directory. + + Other parameters sepcify the polling interval, and the retry
"sepcify"?
+ interval used when a temporary failure is detected. Lock links + can also be cleared on server start.
<mail:queuedDelivery permission="zope.SendMail" queuePath="./queue" - mailer="smtp" /> + retryInterval="300" + pollingInterval="3000" + cleanLockLinks="False" + mailer="smtp" /> -->
Please avoid literal tab characters in source files.
Index: src/zope/sendmail/zcml.py =================================================================== --- src/zope/sendmail/zcml.py (revision 84550) +++ src/zope/sendmail/zcml.py (revision 84551) @@ -70,7 +70,35 @@ class IQueuedDeliveryDirective(IDelivery description=u"Defines the path for the queue directory.", required=True)
-def queuedDelivery(_context, permission, queuePath, mailer, name="Mail"): + pollingInterval = Int( + title=u'Polling Interval', + description=u'Defines the polling interval for queue processing in' + ' milliseconds', + default=3000, + required=False) ... - thread = QueueProcessorThread() + thread = QueueProcessorThread(interval=pollingInterval/1000,
You're using integer division here and losing sub-second precision. If you don't want that, write interval=pollingInterval/1000.0.
+ retry_interval=retryInterval, + clean_lock_links=cleanLockLinks) thread.setMailer(mailerObject) thread.setQueuePath(queuePath) thread.start()
Index: src/zope/sendmail/mailer.py =================================================================== --- src/zope/sendmail/mailer.py (revision 84550) +++ src/zope/sendmail/mailer.py (revision 84551) ... @@ -41,22 +49,105 @@ class SMTPMailer(object): self.password = password self.force_tls = force_tls self.no_tls = no_tls + self.logger = None
- def send(self, fromaddr, toaddrs, message): - connection = self.smtp(self.hostname, str(self.port)) + def set_logger(self, logger): + if not isinstance(logger, logging.Logger): + raise DoesNotImplement('Invalid logger given')
I don't think DoesNotImplement is right here -- there are no interfaces you're looking for. Better use the built-in TypeError. But is the type check necessary at all? The Pythonic thing is to rely on duck typing.
+ self.logger = logger + + def _log(self, log_level, *args): + if self.logger is None: + return + # This is not elegant, but it can be fixed later
In my experience "later" usually doesn't happen. Why not fix it now?
+ if log_level == 'debug': + self.logger.debug(*args) + elif log_level =='info': + self.logger.info(*args) + elif log_level =='error': + self.logger.error(*args) + elif log_level =='warning': + self.logger.warning(*args) +
Just use self.logger.log(log_level, *args) and logging.DEBUG/logging.INFO/logging.ERROR/logging.WARNING as the values for log_level.
+ def _handle_smtp_error(self, + smtp_code, + smtp_error, + fromaddr, + toaddrs, + message_id): + """Process results of an SMTP error + returns True to indicate break needed"""
The standard docstring convention is to do this: """Process results of an SMTP error. Returns True to indicate break needed. """ One sentence on the first line, then an empty line, then additional paragraphs after that. The closing """ on a separate line. All sentences terminated by periods.
+ def send(self, fromaddr, toaddrs, message, message_id): + try: + connection = self.smtp(self.hostname, str(self.port)) + except socket.error, e: + self._log('info', + "%s - SMTP server %s:%s - could not connect()," + " %s.", + message_id, + self.hostname, + str(self.port), + str(e),) + # temporary failure, go and sleep for + # retry_interval + raise MailerTemporaryFailureException()
# send EHLO code, response = connection.ehlo() if code < 200 or code >= 300: code, response = connection.helo() if code < 200 or code >= 300: - raise RuntimeError('Error sending HELO to the SMTP server ' - '(code=%s, response=%s)' % (code, response)) + self._log('warning', + SMTP_ERR_MEDIUM_LOG_MSG, + message_id, + code, + str(response), + 'error sending HELO') + raise MailerTemporaryFailureException()
# encryption support - have_tls = connection.has_extn('starttls') + have_tls = connection.has_extn('starttls') if not have_tls and self.force_tls: - raise RuntimeError('TLS is not available but TLS is required') + error_str = 'TLS is not available but TLS is required' + self._log('warning', + SMTP_ERR_SERVER_LOG_MSG, + message_id, + self.hostname, + self.port, + error_str) + raise MaileriTemporaryFailure(error_str)
Typo: "Mailer*i*TemporaryFailure". Also an indication that this code branch is not tested, otherwise the typo would've been caught.
+ except smtplib.SMTPRecipientsRefused, e: + # This exception is raised because no recipients + # were acceptable - lets take the most common error + # code and proceed with that + freq = {} + for result in e.recipients.values(): + if freq.has_key(result): + freq[result] += 1 + else: + freq[result] = 1 + max_ = 0 + for result in freq.keys(): + if freq[result] > max_: + most_common = result + max_ = freq[result] + (smtp_code, smtp_error) = most_common
Please extract this bit into a separate function. The send method is already way too long and complicated. Once it's extracted an tested with an isolated test, you can refactor it to something like freq = collections.defaultdict(int) for smtp_code, smtp_error in e.recipients.values(): freq[smtp_code, smtp_error] += 1 smtp_code, smtp_error = max(freq, key=freq.get) Oh, wait, that only works on Python 2.5. Scratch the refactoring.
+ # Log ANY errors + if send_errors is not None:
If an exception happens in this bit above:
+ try: + send_errors = connection.sendmail(fromaddr, toaddrs, message) + except smtplib.SMTPSenderRefused, e:
then send_errors will be undefined and you'll get a NameError. Currently this is mostly not a problem, because almost all except clauses for that try re-raise a different exception (either directly, or via _handle_smtp_error). I suggest adding send_errors = None above the try:, just in case.
+ # Log ANY errors + if send_errors is not None: + sentaddrs = [x for x in toaddrs + if x not in send_errors.keys()] + for address in send_errors.keys(): + self._log('warning', + SMTP_ERR_LOG_MSG, + message_id, + str(send_errors[address][0]), + send_errors[address][1], + fromaddr, + address)
Suggest rewriting as sentaddrs = [x for x in toaddrs if x not in send_errors] for address, (smtp_code, smtp_error) in send_errors.items(): self._log('warning', SMTP_ERR_LOG_MSG, message_id, str(smtp_code), smtp_error, fromaddr, address) (Also note that you don't have to explicitly call str() on all ints you pass to a message; '%s' % 42 works perfectly fine.)
+ else: + sentaddrs = list(toaddrs)
Index: src/zope/sendmail/tests/test_mailer.py =================================================================== --- src/zope/sendmail/tests/test_mailer.py (revision 84550) +++ src/zope/sendmail/tests/test_mailer.py (revision 84551) ... class SMTP(object): + _exception = None + _exception_args = None + _send_errors = None
def __init__(myself, h, p): myself.hostname = h myself.port = p + self.smtp = myself if type(p) == type(u""): raise socket.error("Int or String expected") - self.smtp = myself + if p == '0': + raise socket.error("Emulated connect() failure")
def sendmail(self, f, t, m): self.fromaddr = f self.toaddrs = t self.msgtext = m + if hasattr(self, '_exception'): + if self._exception and issubclass(self._exception, Exception): + if hasattr(self, '_exception_args') \ + and type(self._exception_args) is tuple: + raise self._exception(*self._exception_args) + else: + raise self._exception('Crazy Arguments WANTED!')
That's a bit of an overkill. If you change the _exception_args default value to () above, it would be perfectly fine to replace this lats bit with if self._exception: raise self._exception(*self._exception_args) It's the job of the Python interpreter to check the suitability of _exception and _exception_args for the raise statement, you don't have to do everything by hand.
+ if self._send_errors: + return self._send_errors ... + def test_set_logger(self): + # Do this with throw away instances... + test_mailer = SMTPMailer() + log_object = logging.getLogger('test_logger') + test_mailer.set_logger(log_object) + self.assertEquals(isinstance(test_mailer.logger, logging.Logger), True)
Why not self.assertTrue(test_mailer.logger is log_object) instead?
def test_send(self): for run in (1,2): if run == 2: self.setUp(u'25') - fromaddr = 'me@example.com' - toaddrs = ('you@example.com', 'him@example.com') - msgtext = 'Headers: headers\n\nbodybodybody\n-- \nsig\n' - self.mailer.send(fromaddr, toaddrs, msgtext) - self.assertEquals(self.smtp.fromaddr, fromaddr) - self.assertEquals(self.smtp.toaddrs, toaddrs) - self.assertEquals(self.smtp.msgtext, msgtext) + else: + self.setUp()
Pointless, the unittest framework calls self.setUp() for you automatically.
+ def test_mailer_no_connect(self): + # set up test value to raise socket.error exception + self.setUp('0') + try: + self.mailer.send(**self.test_kwargs) + except MailerTemporaryFailureException: + pass
You want self.assertRaises(MailerTemporaryFailureException, self.mailer.send, **self.test_kwargs) here (and elsewhere)
+ self.assertEquals(self.mailer.logger.infos,
Index: src/zope/sendmail/tests/test_delivery.py =================================================================== --- src/zope/sendmail/tests/test_delivery.py (revision 84550) +++ src/zope/sendmail/tests/test_delivery.py (revision 84551) @@ -21,6 +21,7 @@ $Id$ ... +from socket import error as socket_error
What's the advantage of this over import socket and then using socket.error elsewhere?
+ def test_unlink(self): + self.thread.log = LoggerStub() # Clean log + self.filename = os.path.join(self.dir, 'message') + self.tmp_filename = os.path.join(self.dir, '.sending-message') + temp = open(self.filename, "w+b") + temp.write('X-Zope-From: foo@example.com\n' + 'X-Zope-To: bar@example.com, baz@example.com\n' + 'Header: value\n\nBody\n') + temp.close() + self.md.files.append(self.filename) + os.link(self.filename, self.tmp_filename)
Is os.link available on Windows?
+ self.thread._unlinkFile(self.tmp_filename) + self.failUnless(os.path.exists(self.filename)) + self.failIf(os.path.exists(self.tmp_filename), 'File exists')
Index: src/zope/sendmail/maildir.py =================================================================== --- src/zope/sendmail/maildir.py (revision 84550) +++ src/zope/sendmail/maildir.py (revision 84551) ... + def _cleanLockLinks(self): + """Clean the maildir of any .sending-* lock files""" + # Fish inside the Maildir queue directory to get + # .sending-* files + # Get The links + join = os.path.join + subdir_cur = join(self.path, 'cur') + subdir_new = join(self.path, 'new') + lock_links = [join(subdir_new, x) for x in os.listdir(subdir_new) + if x.startswith(SENDING_MSG_LOCK_PREFIX)] + lock_links += [join(subdir_cur, x) for x in os.listdir(subdir_cur) + if x.startswith(SENDING_MSG_LOCK_PREFIX)] + # Remove any links + for link in lock_links: + try: + os.unlink(link) + except OSError, e: + if e.errno == 2: # file does not exist
If you import errno, you can write if e.errno = errno.ENOENT: instead. I find that a bit clearer.
+ # someone else unlinked the file; oh well + pass + else: + # something bad happend + raise
Index: src/zope/sendmail/interfaces.py =================================================================== --- src/zope/sendmail/interfaces.py (revision 84550) +++ src/zope/sendmail/interfaces.py (revision 84551) ... +# +# Exception classes for use within Zope Sendmail, for use of Mailers +# +class IMailerFailureException(IException): + """Failure in sending mail""" + pass
The 'pass' is not needed here.
+class MailerFailureException(Exception): + """Failure in sending mail""" + + implements(IMailerFailureException) + + def __init__(self, message="Failure in sending mail"): + self.message = message + self.args = (message,)
Is the __init__ needed? Exception.__init__ will do essentially the same thing.
+ +class IMailerTemporaryFailureException(IMailerFailureException): + """Temporary failure in sending mail - retry later""" + pass + +class MailerTemporaryFailureException(MailerFailureException): + """Temporary failure in sending mail - retry later""" + + implements(IMailerTemporaryFailureException) + + def __init__(self, message="Temporary failure in sending mail - retry later"): + self.message = message + self.args = (message,) + + +class IMailerPermanentFailureException(IMailerFailureException): + """Permanent failure in sending mail - take reject action""" + pass + +class MailerPermanentFailureException(MailerFailureException): + """Permanent failure in sending mail - take reject action""" + + implements(IMailerPermanentFailureException) + + def __init__(self, message="Permanent failure in sending mail - take reject action"): + self.message = message + self.args = (message,) +
class IMailer(Interface): - """Mailer handles synchronous mail delivery.""" + """Mailer handles synchronous mail delivery.
- def send(fromaddr, toaddrs, message): + Mailer can raise the exceptions + + MailerPermanentFailure + MailerTemporaryFailure
Well, no, your exception classes are named MailerTemporaryFailureException and MailerPermanentFailureException. (I'd prefer the shorter names, personally. Maybe MailerTemporaryError and MailerPermanentError, because FooError is the common naming convention for exceptions.)
+ + to indicate to sending process what action to take. + """ + + def send(fromaddr, toaddrs, message, message_id): """Send an email message.
`fromaddr` is the sender address (unicode string), @@ -138,12 +203,18 @@ class IMailer(Interface): 2822. It should contain at least Date, From, To, and Message-Id headers.
+ `message_id` is an id for the message, typically a filename. +
The naming clash with the Message-ID header (which has got *absolutely nothing* to do with this message_id argument) is unfortunate. I think this should be clarified in the docstring. No, I think someone should go through the source code and replace message_id with queue_id everywhere where it refers to the os.path.basename(file_in_queue) and not to the Message-ID header.
Messages are sent immediatelly.
Dispatches an `IMailSentEvent` on successful delivery, otherwise an `IMailErrorEvent`. """
+ def set_logger(logger): + """Set the log object for the Mailer - this is for use by + QueueProcessorThread to hand a logging object to the mailer + """
Docstring formatting convention. """Set the logger for additional messages. The additional messages include information about SMTP errors, connection timeouts, etc. The logger should be a logging.Logger object. If you pass None, no messages will be logged. """ Also, it's conventional to leave two blank lines between class and interface declarations. (PEP-8 says this.)
class ISMTPMailer(IMailer): """A mailer that delivers mail to a relay host via SMTP."""
Index: src/zope/sendmail/delivery.py =================================================================== --- src/zope/sendmail/delivery.py (revision 84550) +++ src/zope/sendmail/delivery.py (revision 84551) @@ -223,6 +241,8 @@ class QueueProcessorThread(threading.Thr self.maildir = Maildir(path, True)
def setMailer(self, mailer): + if not(IMailer.providedBy(mailer)): + raise (DoesNotImplement)
This doesn't look like Python -- all those extra parens. if not IMailer.providedBy(mailer): raise TypeError('not an IMailer', mailer)
self.mailer = mailer
def _parseMessage(self, message):
...
+ def _unlinkFile(self, filename): + """Unlink the message file """ + try: + os.unlink(filename) + except OSError, e: + if e.errno == 2: # file does not exist + # someone else unlinked the file; oh well + pass + else: + # something bad happend, log it + raise
I've seen this function before. Can you merge all the copies into a global function, say, called safe_delete()?
+ def _queueRetryWait(self, tmp_filename, forever): + """Implements Retry Wait if there is an SMTP Connection + Failure or error 4xx due to machine load etc
Blank line after the first one, then no extra indentation.
+ """ + # Clean up by unlinking lock link + self._unlinkFile(tmp_filename) + # Wait specified retry interval in stages of self.interval + count = self.retry_interval + while(count > 0 and not self.__stopped): + if forever: + time.sleep(self.interval) + count -= self.interval
Interesting. But lose the unnecessary parens after the while, at least. Is there any point to enter the loop if the forever arg is False?
+ # Plug for test routines so that we know we got here + if not forever: + self.test_results['_queueRetryWait'] \ + = "Retry timeout: %s count: %s" \ + % (str(self.retry_interval), str(count))
Ouch. I think monkeypatching this method would be cleaner.
def run(self, forever=True): atexit.register(self.stop) + # Clean .sending- lock files from queue + if self.clean_lock_links: + self.maildir._cleanLockLinks() + # Set up logger for mailer + if hasattr(self.mailer, 'set_logger'): + self.mailer.set_logger(self.log)
The interface requires all mailers to have a set_logger(), so why the hasattr test?
@@ -263,8 +320,9 @@ class QueueProcessorThread(threading.Thr fromaddr = '' toaddrs = () head, tail = os.path.split(filename) - tmp_filename = os.path.join(head, '.sending-' + tail) - rejected_filename = os.path.join(head, '.rejected-' + tail) + tmp_filename = os.path.join(head, SENDING_MSG_LOCK_PREFIX + tail) + rejected_filename = os.path.join(head, REJECTED_MSG_PREFIX + tail) + message_id = os.path.basename(filename) try: # perform a series of operations in an attempt to ensure # that no two threads/processes send this message @@ -339,53 +397,46 @@ class QueueProcessorThread(threading.Thr file.close() fromaddr, toaddrs, message = self._parseMessage(message) try: - self.mailer.send(fromaddr, toaddrs, message)
Oh, the message_id argument to mailer.send() is new! Please, please, please, call it queue_id, and not message_id.
- except smtplib.SMTPResponseException, e: - if 500 <= e.smtp_code <= 599: - # permanent error, ditch the message - self.log.error( - "Discarding email from %s to %s due to" - " a permanent error: %s", - fromaddr, ", ".join(toaddrs), str(e)) - os.link(filename, rejected_filename) - else: - # Log an error and retry later - raise + sentaddrs = self.mailer.send(fromaddr, + toaddrs, + message, + message_id) + except MailerTemporaryFailureException, e: + self._queueRetryWait(tmp_filename, forever) + # We break as we don't want to send message later + break;
No semicolons please.
+ except MailerPermanentFailureException, e: + os.link(filename, rejected_filename) + sentaddrs = []
Overall I like the new features *a lot*. The stylistic issues are mostly minor. The most important of those are * Fix the integer division bug (easy, but a unit test would be good). * Resolve the exception name confusion (easy). * Clearly distinguish queue IDs from RFC-2822 message IDs (should be easy). And I have a questions: * What's up with retry_interval? _queueRetryWait doesn't do anything in the inner loop, after sleeping for retry_interval seconds -- it just checks self.__stopped. I suppose this makes Zope 3 shutdown (or restart) faster, but the name seems to be wrong -- there are no actual retries made after retry_interval seconds. Thank you very much for diving into the murky depths and improving zope.sendmail! Marius Gedminas -- If it weren't for the last minute, nothing would get done.
Marius Gedminas wrote:
+ """Process results of an SMTP error + returns True to indicate break needed"""
The standard docstring convention is to do this:
"""Process results of an SMTP error.
Returns True to indicate break needed. """
This standard sucks. """ Process results of an SMTP error. Returns True to indicate break needed. """ ...is much more readable. If there's a tool that does something silly with this, fix the tool.
Also, it's conventional to leave two blank lines between class and interface declarations. (PEP-8 says this.)
What does this actually mean? I couldn't follow from your example. Other than that, completely agree with everything you said :-) cheers, Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
On Thu, Mar 20, 2008 at 10:33:10PM +0000, Chris Withers wrote:
Marius Gedminas wrote:
+ """Process results of an SMTP error + returns True to indicate break needed""" The standard docstring convention is to do this: """Process results of an SMTP error.
There was a blank line here that you omitted.
Returns True to indicate break needed. """
This standard sucks.
""" Process results of an SMTP error.
Returns True to indicate break needed. """
...is much more readable. If there's a tool that does something silly with this, fix the tool.
It's a matter of taste, perhaps, because I do find """Short sentence. Longer description. """ nicer than your example. Luckily, PEP-8 agrees with my taste. :-) Okay, I lied. PEP-8 says to add an extra blank line before the closing """ (because Emacs' paragraph wrapping command sucks), and thus you get this: """Short sentence. Longer description. """ but I don't recall ever seeing this variant in the Zope 3 tree. I completely agree with your sentiment (fix the damn tool!) here. My beloved vim can format the longer description without touching the trailing """.
Also, it's conventional to leave two blank lines between class and interface declarations. (PEP-8 says this.)
What does this actually mean? I couldn't follow from your example.
I'll try to give a better example. Count the blank lines in it: ------------------------ class OneClass(object): """Docstring.""" def a_method(self): """Docstring.""" code code code def b_method(self): """Docstring.""" code code class AnotherClass(object): """Docstring.""" def a_method(self): """Docstring.""" code code class ThirdClass(object): """Docstring.""" ------------------------- Class definitions are separated by two blank lines. Method definitions are separated by a single blank line. That's what PEP-8 says.
Other than that, completely agree with everything you said :-)
It's always the little and unimportant things that get the most attention and discussion. On the other hand, at least we have a consensus (PEP-8) in the Python community, unlike those poor C++ brace style flame war veterans. I suppose having no braces helps ;-) Cheers! Marius Gedminas -- The day after tomorrow is the third day of the rest of your life.
Working on this. tehre are some things I will get back to you about, because what you suggest breaks the unit test code... Cheers, Matthew On Wed, 2008-03-19 at 23:32 +0200, Marius Gedminas wrote:
I'm starting a new thread, because the original one got hijacked by the doctest/unittest flamefest.
I appear to have unsubscribed from zope3-checkins a while ago, which is why I'm posting here.
Disclaimer: I'm not very good at doing code reviews. I tend to mention every little detail that could've been improved and forget to praise everything that's already perfect.
There are two checkins on the branch.
========= rev 84645 ========= http://svn.zope.org/zope.sendmail/branches/grantma-retryfixes/?rev=84645&vie...
Matthew, Subversion thinks the two PDF files you added are plain-text (svn:eol-style is "native", and svn diff shows the actual contents of the PDFs, which isn't very useful to human readers). I would suggest
svn propdel svn:eol-style *.pdf svn propset svn:mime-type application/pdf *.pdf
Also, the Quue_*.pdf seems to have a typo in the file name.
At first sight those diagrams look scary and complicated. I'd look into ways to express their content in a text file with plain-text explanations and ASCII-art diagrams.
========= rev 84551 ========= http://svn.zope.org/zope.sendmail/branches/grantma-retryfixes/?rev=84551&vie...
Index: src/zope/sendmail/configure.zcml =================================================================== --- src/zope/sendmail/configure.zcml (revision 84550) +++ src/zope/sendmail/configure.zcml (revision 84551) @@ -14,11 +14,18 @@
<!-- To send mail, uncomment the following directive and be sure to - create the queue directory. + create the queue directory. + + Other parameters sepcify the polling interval, and the retry
"sepcify"?
+ interval used when a temporary failure is detected. Lock links + can also be cleared on server start.
<mail:queuedDelivery permission="zope.SendMail" queuePath="./queue" - mailer="smtp" /> + retryInterval="300" + pollingInterval="3000" + cleanLockLinks="False" + mailer="smtp" /> -->
Please avoid literal tab characters in source files.
Index: src/zope/sendmail/zcml.py =================================================================== --- src/zope/sendmail/zcml.py (revision 84550) +++ src/zope/sendmail/zcml.py (revision 84551) @@ -70,7 +70,35 @@ class IQueuedDeliveryDirective(IDelivery description=u"Defines the path for the queue directory.", required=True)
-def queuedDelivery(_context, permission, queuePath, mailer, name="Mail"): + pollingInterval = Int( + title=u'Polling Interval', + description=u'Defines the polling interval for queue processing in' + ' milliseconds', + default=3000, + required=False) ... - thread = QueueProcessorThread() + thread = QueueProcessorThread(interval=pollingInterval/1000,
You're using integer division here and losing sub-second precision. If you don't want that, write interval=pollingInterval/1000.0.
+ retry_interval=retryInterval, + clean_lock_links=cleanLockLinks) thread.setMailer(mailerObject) thread.setQueuePath(queuePath) thread.start()
Index: src/zope/sendmail/mailer.py =================================================================== --- src/zope/sendmail/mailer.py (revision 84550) +++ src/zope/sendmail/mailer.py (revision 84551) ... @@ -41,22 +49,105 @@ class SMTPMailer(object): self.password = password self.force_tls = force_tls self.no_tls = no_tls + self.logger = None
- def send(self, fromaddr, toaddrs, message): - connection = self.smtp(self.hostname, str(self.port)) + def set_logger(self, logger): + if not isinstance(logger, logging.Logger): + raise DoesNotImplement('Invalid logger given')
I don't think DoesNotImplement is right here -- there are no interfaces you're looking for. Better use the built-in TypeError.
But is the type check necessary at all? The Pythonic thing is to rely on duck typing.
+ self.logger = logger + + def _log(self, log_level, *args): + if self.logger is None: + return + # This is not elegant, but it can be fixed later
In my experience "later" usually doesn't happen. Why not fix it now?
+ if log_level == 'debug': + self.logger.debug(*args) + elif log_level =='info': + self.logger.info(*args) + elif log_level =='error': + self.logger.error(*args) + elif log_level =='warning': + self.logger.warning(*args) +
Just use
self.logger.log(log_level, *args)
and logging.DEBUG/logging.INFO/logging.ERROR/logging.WARNING as the values for log_level.
+ def _handle_smtp_error(self, + smtp_code, + smtp_error, + fromaddr, + toaddrs, + message_id): + """Process results of an SMTP error + returns True to indicate break needed"""
The standard docstring convention is to do this:
"""Process results of an SMTP error.
Returns True to indicate break needed. """
One sentence on the first line, then an empty line, then additional paragraphs after that. The closing """ on a separate line. All sentences terminated by periods.
+ def send(self, fromaddr, toaddrs, message, message_id): + try: + connection = self.smtp(self.hostname, str(self.port)) + except socket.error, e: + self._log('info', + "%s - SMTP server %s:%s - could not connect()," + " %s.", + message_id, + self.hostname, + str(self.port), + str(e),) + # temporary failure, go and sleep for + # retry_interval + raise MailerTemporaryFailureException()
# send EHLO code, response = connection.ehlo() if code < 200 or code >= 300: code, response = connection.helo() if code < 200 or code >= 300: - raise RuntimeError('Error sending HELO to the SMTP server ' - '(code=%s, response=%s)' % (code, response)) + self._log('warning', + SMTP_ERR_MEDIUM_LOG_MSG, + message_id, + code, + str(response), + 'error sending HELO') + raise MailerTemporaryFailureException()
# encryption support - have_tls = connection.has_extn('starttls') + have_tls = connection.has_extn('starttls') if not have_tls and self.force_tls: - raise RuntimeError('TLS is not available but TLS is required') + error_str = 'TLS is not available but TLS is required' + self._log('warning', + SMTP_ERR_SERVER_LOG_MSG, + message_id, + self.hostname, + self.port, + error_str) + raise MaileriTemporaryFailure(error_str)
Typo: "Mailer*i*TemporaryFailure". Also an indication that this code branch is not tested, otherwise the typo would've been caught.
+ except smtplib.SMTPRecipientsRefused, e: + # This exception is raised because no recipients + # were acceptable - lets take the most common error + # code and proceed with that + freq = {} + for result in e.recipients.values(): + if freq.has_key(result): + freq[result] += 1 + else: + freq[result] = 1 + max_ = 0 + for result in freq.keys(): + if freq[result] > max_: + most_common = result + max_ = freq[result] + (smtp_code, smtp_error) = most_common
Please extract this bit into a separate function. The send method is already way too long and complicated.
Once it's extracted an tested with an isolated test, you can refactor it to something like
freq = collections.defaultdict(int) for smtp_code, smtp_error in e.recipients.values(): freq[smtp_code, smtp_error] += 1 smtp_code, smtp_error = max(freq, key=freq.get)
Oh, wait, that only works on Python 2.5. Scratch the refactoring.
+ # Log ANY errors + if send_errors is not None:
If an exception happens in this bit above:
+ try: + send_errors = connection.sendmail(fromaddr, toaddrs, message) + except smtplib.SMTPSenderRefused, e:
then send_errors will be undefined and you'll get a NameError. Currently this is mostly not a problem, because almost all except clauses for that try re-raise a different exception (either directly, or via _handle_smtp_error).
I suggest adding
send_errors = None
above the try:, just in case.
+ # Log ANY errors + if send_errors is not None: + sentaddrs = [x for x in toaddrs + if x not in send_errors.keys()] + for address in send_errors.keys(): + self._log('warning', + SMTP_ERR_LOG_MSG, + message_id, + str(send_errors[address][0]), + send_errors[address][1], + fromaddr, + address)
Suggest rewriting as
sentaddrs = [x for x in toaddrs if x not in send_errors] for address, (smtp_code, smtp_error) in send_errors.items(): self._log('warning', SMTP_ERR_LOG_MSG, message_id, str(smtp_code), smtp_error, fromaddr, address)
(Also note that you don't have to explicitly call str() on all ints you pass to a message; '%s' % 42 works perfectly fine.)
+ else: + sentaddrs = list(toaddrs)
Index: src/zope/sendmail/tests/test_mailer.py =================================================================== --- src/zope/sendmail/tests/test_mailer.py (revision 84550) +++ src/zope/sendmail/tests/test_mailer.py (revision 84551) ... class SMTP(object): + _exception = None + _exception_args = None + _send_errors = None
def __init__(myself, h, p): myself.hostname = h myself.port = p + self.smtp = myself if type(p) == type(u""): raise socket.error("Int or String expected") - self.smtp = myself + if p == '0': + raise socket.error("Emulated connect() failure")
def sendmail(self, f, t, m): self.fromaddr = f self.toaddrs = t self.msgtext = m + if hasattr(self, '_exception'): + if self._exception and issubclass(self._exception, Exception): + if hasattr(self, '_exception_args') \ + and type(self._exception_args) is tuple: + raise self._exception(*self._exception_args) + else: + raise self._exception('Crazy Arguments WANTED!')
That's a bit of an overkill. If you change the _exception_args default value to () above, it would be perfectly fine to replace this lats bit with
if self._exception: raise self._exception(*self._exception_args)
It's the job of the Python interpreter to check the suitability of _exception and _exception_args for the raise statement, you don't have to do everything by hand.
+ if self._send_errors: + return self._send_errors ... + def test_set_logger(self): + # Do this with throw away instances... + test_mailer = SMTPMailer() + log_object = logging.getLogger('test_logger') + test_mailer.set_logger(log_object) + self.assertEquals(isinstance(test_mailer.logger, logging.Logger), True)
Why not
self.assertTrue(test_mailer.logger is log_object)
instead?
def test_send(self): for run in (1,2): if run == 2: self.setUp(u'25') - fromaddr = 'me@example.com' - toaddrs = ('you@example.com', 'him@example.com') - msgtext = 'Headers: headers\n\nbodybodybody\n-- \nsig\n' - self.mailer.send(fromaddr, toaddrs, msgtext) - self.assertEquals(self.smtp.fromaddr, fromaddr) - self.assertEquals(self.smtp.toaddrs, toaddrs) - self.assertEquals(self.smtp.msgtext, msgtext) + else: + self.setUp()
Pointless, the unittest framework calls self.setUp() for you automatically.
+ def test_mailer_no_connect(self): + # set up test value to raise socket.error exception + self.setUp('0') + try: + self.mailer.send(**self.test_kwargs) + except MailerTemporaryFailureException: + pass
You want
self.assertRaises(MailerTemporaryFailureException, self.mailer.send, **self.test_kwargs)
here (and elsewhere)
+ self.assertEquals(self.mailer.logger.infos,
Index: src/zope/sendmail/tests/test_delivery.py =================================================================== --- src/zope/sendmail/tests/test_delivery.py (revision 84550) +++ src/zope/sendmail/tests/test_delivery.py (revision 84551) @@ -21,6 +21,7 @@ $Id$ ... +from socket import error as socket_error
What's the advantage of this over
import socket
and then using socket.error elsewhere?
+ def test_unlink(self): + self.thread.log = LoggerStub() # Clean log + self.filename = os.path.join(self.dir, 'message') + self.tmp_filename = os.path.join(self.dir, '.sending-message') + temp = open(self.filename, "w+b") + temp.write('X-Zope-From: foo@example.com\n' + 'X-Zope-To: bar@example.com, baz@example.com\n' + 'Header: value\n\nBody\n') + temp.close() + self.md.files.append(self.filename) + os.link(self.filename, self.tmp_filename)
Is os.link available on Windows?
+ self.thread._unlinkFile(self.tmp_filename) + self.failUnless(os.path.exists(self.filename)) + self.failIf(os.path.exists(self.tmp_filename), 'File exists')
Index: src/zope/sendmail/maildir.py =================================================================== --- src/zope/sendmail/maildir.py (revision 84550) +++ src/zope/sendmail/maildir.py (revision 84551) ... + def _cleanLockLinks(self): + """Clean the maildir of any .sending-* lock files""" + # Fish inside the Maildir queue directory to get + # .sending-* files + # Get The links + join = os.path.join + subdir_cur = join(self.path, 'cur') + subdir_new = join(self.path, 'new') + lock_links = [join(subdir_new, x) for x in os.listdir(subdir_new) + if x.startswith(SENDING_MSG_LOCK_PREFIX)] + lock_links += [join(subdir_cur, x) for x in os.listdir(subdir_cur) + if x.startswith(SENDING_MSG_LOCK_PREFIX)] + # Remove any links + for link in lock_links: + try: + os.unlink(link) + except OSError, e: + if e.errno == 2: # file does not exist
If you import errno, you can write
if e.errno = errno.ENOENT:
instead. I find that a bit clearer.
+ # someone else unlinked the file; oh well + pass + else: + # something bad happend + raise
Index: src/zope/sendmail/interfaces.py =================================================================== --- src/zope/sendmail/interfaces.py (revision 84550) +++ src/zope/sendmail/interfaces.py (revision 84551) ... +# +# Exception classes for use within Zope Sendmail, for use of Mailers +# +class IMailerFailureException(IException): + """Failure in sending mail""" + pass
The 'pass' is not needed here.
+class MailerFailureException(Exception): + """Failure in sending mail""" + + implements(IMailerFailureException) + + def __init__(self, message="Failure in sending mail"): + self.message = message + self.args = (message,)
Is the __init__ needed? Exception.__init__ will do essentially the same thing.
+ +class IMailerTemporaryFailureException(IMailerFailureException): + """Temporary failure in sending mail - retry later""" + pass + +class MailerTemporaryFailureException(MailerFailureException): + """Temporary failure in sending mail - retry later""" + + implements(IMailerTemporaryFailureException) + + def __init__(self, message="Temporary failure in sending mail - retry later"): + self.message = message + self.args = (message,) + + +class IMailerPermanentFailureException(IMailerFailureException): + """Permanent failure in sending mail - take reject action""" + pass + +class MailerPermanentFailureException(MailerFailureException): + """Permanent failure in sending mail - take reject action""" + + implements(IMailerPermanentFailureException) + + def __init__(self, message="Permanent failure in sending mail - take reject action"): + self.message = message + self.args = (message,) +
class IMailer(Interface): - """Mailer handles synchronous mail delivery.""" + """Mailer handles synchronous mail delivery.
- def send(fromaddr, toaddrs, message): + Mailer can raise the exceptions + + MailerPermanentFailure + MailerTemporaryFailure
Well, no, your exception classes are named MailerTemporaryFailureException and MailerPermanentFailureException.
(I'd prefer the shorter names, personally. Maybe MailerTemporaryError and MailerPermanentError, because FooError is the common naming convention for exceptions.)
+ + to indicate to sending process what action to take. + """ + + def send(fromaddr, toaddrs, message, message_id): """Send an email message.
`fromaddr` is the sender address (unicode string), @@ -138,12 +203,18 @@ class IMailer(Interface): 2822. It should contain at least Date, From, To, and Message-Id headers.
+ `message_id` is an id for the message, typically a filename. +
The naming clash with the Message-ID header (which has got *absolutely nothing* to do with this message_id argument) is unfortunate. I think this should be clarified in the docstring. No, I think someone should go through the source code and replace message_id with queue_id everywhere where it refers to the os.path.basename(file_in_queue) and not to the Message-ID header.
Messages are sent immediatelly.
Dispatches an `IMailSentEvent` on successful delivery, otherwise an `IMailErrorEvent`. """
+ def set_logger(logger): + """Set the log object for the Mailer - this is for use by + QueueProcessorThread to hand a logging object to the mailer + """
Docstring formatting convention.
"""Set the logger for additional messages.
The additional messages include information about SMTP errors, connection timeouts, etc.
The logger should be a logging.Logger object. If you pass None, no messages will be logged. """
Also, it's conventional to leave two blank lines between class and interface declarations. (PEP-8 says this.)
class ISMTPMailer(IMailer): """A mailer that delivers mail to a relay host via SMTP."""
Index: src/zope/sendmail/delivery.py =================================================================== --- src/zope/sendmail/delivery.py (revision 84550) +++ src/zope/sendmail/delivery.py (revision 84551) @@ -223,6 +241,8 @@ class QueueProcessorThread(threading.Thr self.maildir = Maildir(path, True)
def setMailer(self, mailer): + if not(IMailer.providedBy(mailer)): + raise (DoesNotImplement)
This doesn't look like Python -- all those extra parens.
if not IMailer.providedBy(mailer): raise TypeError('not an IMailer', mailer)
self.mailer = mailer
def _parseMessage(self, message):
...
+ def _unlinkFile(self, filename): + """Unlink the message file """ + try: + os.unlink(filename) + except OSError, e: + if e.errno == 2: # file does not exist + # someone else unlinked the file; oh well + pass + else: + # something bad happend, log it + raise
I've seen this function before. Can you merge all the copies into a global function, say, called safe_delete()?
+ def _queueRetryWait(self, tmp_filename, forever): + """Implements Retry Wait if there is an SMTP Connection + Failure or error 4xx due to machine load etc
Blank line after the first one, then no extra indentation.
+ """ + # Clean up by unlinking lock link + self._unlinkFile(tmp_filename) + # Wait specified retry interval in stages of self.interval + count = self.retry_interval + while(count > 0 and not self.__stopped): + if forever: + time.sleep(self.interval) + count -= self.interval
Interesting. But lose the unnecessary parens after the while, at least.
Is there any point to enter the loop if the forever arg is False?
+ # Plug for test routines so that we know we got here + if not forever: + self.test_results['_queueRetryWait'] \ + = "Retry timeout: %s count: %s" \ + % (str(self.retry_interval), str(count))
Ouch. I think monkeypatching this method would be cleaner.
def run(self, forever=True): atexit.register(self.stop) + # Clean .sending- lock files from queue + if self.clean_lock_links: + self.maildir._cleanLockLinks() + # Set up logger for mailer + if hasattr(self.mailer, 'set_logger'): + self.mailer.set_logger(self.log)
The interface requires all mailers to have a set_logger(), so why the hasattr test?
@@ -263,8 +320,9 @@ class QueueProcessorThread(threading.Thr fromaddr = '' toaddrs = () head, tail = os.path.split(filename) - tmp_filename = os.path.join(head, '.sending-' + tail) - rejected_filename = os.path.join(head, '.rejected-' + tail) + tmp_filename = os.path.join(head, SENDING_MSG_LOCK_PREFIX + tail) + rejected_filename = os.path.join(head, REJECTED_MSG_PREFIX + tail) + message_id = os.path.basename(filename) try: # perform a series of operations in an attempt to ensure # that no two threads/processes send this message @@ -339,53 +397,46 @@ class QueueProcessorThread(threading.Thr file.close() fromaddr, toaddrs, message = self._parseMessage(message) try: - self.mailer.send(fromaddr, toaddrs, message)
Oh, the message_id argument to mailer.send() is new! Please, please, please, call it queue_id, and not message_id.
- except smtplib.SMTPResponseException, e: - if 500 <= e.smtp_code <= 599: - # permanent error, ditch the message - self.log.error( - "Discarding email from %s to %s due to" - " a permanent error: %s", - fromaddr, ", ".join(toaddrs), str(e)) - os.link(filename, rejected_filename) - else: - # Log an error and retry later - raise + sentaddrs = self.mailer.send(fromaddr, + toaddrs, + message, + message_id) + except MailerTemporaryFailureException, e: + self._queueRetryWait(tmp_filename, forever) + # We break as we don't want to send message later + break;
No semicolons please.
+ except MailerPermanentFailureException, e: + os.link(filename, rejected_filename) + sentaddrs = []
Overall I like the new features *a lot*. The stylistic issues are mostly minor. The most important of those are
* Fix the integer division bug (easy, but a unit test would be good).
* Resolve the exception name confusion (easy).
* Clearly distinguish queue IDs from RFC-2822 message IDs (should be easy).
And I have a questions:
* What's up with retry_interval? _queueRetryWait doesn't do anything in the inner loop, after sleeping for retry_interval seconds -- it just checks self.__stopped. I suppose this makes Zope 3 shutdown (or restart) faster, but the name seems to be wrong -- there are no actual retries made after retry_interval seconds.
Thank you very much for diving into the murky depths and improving zope.sendmail!
Marius Gedminas
Hi! Just done edits you suggested. Some Unit tests still to be written. Won't be doing one for that /1000 as tehre are more important things to check like exception functionality. On Wed, 2008-03-19 at 23:32 +0200, Marius Gedminas wrote:
There are two checkins on the branch.
========= rev 84645 ========= http://svn.zope.org/zope.sendmail/branches/grantma-retryfixes/?rev=84645&vie...
Matthew, Subversion thinks the two PDF files you added are plain-text (svn:eol-style is "native", and svn diff shows the actual contents of the PDFs, which isn't very useful to human readers). I would suggest
svn propdel svn:eol-style *.pdf svn propset svn:mime-type application/pdf *.pdf
Also, the Quue_*.pdf seems to have a typo in the file name.
At first sight those diagrams look scary and complicated. I'd look into ways to express their content in a text file with plain-text explanations and ASCII-art diagrams.
I am paid to write code, not spend hours messing with documentation tools. Scans of my pencil diagrams are all that I can do because of time constraints. Better than nothing.
Index: src/zope/sendmail/tests/test_mailer.py
=================================================================== --- src/zope/sendmail/tests/test_mailer.py (revision 84550) +++ src/zope/sendmail/tests/test_mailer.py (revision 84551)
def sendmail(self, f, t, m): self.fromaddr = f self.toaddrs = t self.msgtext = m + if hasattr(self, '_exception'): + if self._exception and issubclass(self._exception, Exception): + if hasattr(self, '_exception_args') \ + and type(self._exception_args) is tuple: + raise self._exception(*self._exception_args) + else: + raise self._exception('Crazy Arguments WANTED!')
That's a bit of an overkill. If you change the _exception_args default value to () above, it would be perfectly fine to replace this lats bit with
if self._exception: raise self._exception(*self._exception_args)
It's the job of the Python interpreter to check the suitability of _exception and _exception_args for the raise statement, you don't have to do everything by hand.
Left this as I can't get what you suggest to work - this is only test code. I think this is there because this stub is used in many places,and this saves gobs of repeated test code. Due to time I can't look any further into this.
+ def test_unlink(self): + self.thread.log = LoggerStub() # Clean log + self.filename = os.path.join(self.dir, 'message') + self.tmp_filename = os.path.join(self.dir, '.sending-message') + temp = open(self.filename, "w+b") + temp.write('X-Zope-From: foo@example.com\n' + 'X-Zope-To: bar@example.com, baz@example.com\n' + 'Header: value\n\nBody\n') + temp.close() + self.md.files.append(self.filename) + os.link(self.filename, self.tmp_filename)
Is os.link available on Windows?
Don't think it is natively.... but they may have an abortion/aberrative thing to POSIXise the API!
Index: src/zope/sendmail/delivery.py =================================================================== --- src/zope/sendmail/delivery.py (revision 84550) +++ src/zope/sendmail/delivery.py (revision 84551) ... + def _unlinkFile(self, filename): + """Unlink the message file """ + try: + os.unlink(filename) + except OSError, e: + if e.errno == 2: # file does not exist + # someone else unlinked the file; oh well + pass + else: + # something bad happend, log it + raise
I've seen this function before. Can you merge all the copies into a global function, say, called safe_delete()?
You probably have seen this in another part of delivery.py that is very similar? Around os.link etc.... I have taken out all the exatly common code to that above if read delivery.py closely.
+ def _queueRetryWait(self, tmp_filename, forever): + """Implements Retry Wait if there is an SMTP Connection + Failure or error 4xx due to machine load etc
Blank line after the first one, then no extra indentation.
+ """ + # Clean up by unlinking lock link + self._unlinkFile(tmp_filename) + # Wait specified retry interval in stages of self.interval + count = self.retry_interval + while(count > 0 and not self.__stopped): + if forever: + time.sleep(self.interval) + count -= self.interval
Interesting. But lose the unnecessary parens after the while, at least.
Is there any point to enter the loop if the forever arg is False?
+ # Plug for test routines so that we know we got here + if not forever: + self.test_results['_queueRetryWait'] \ + = "Retry timeout: %s count: %s" \ + % (str(self.retry_interval), str(count))
Ouch. I think monkeypatching this method would be cleaner.
We want to test that this delay code is functional or we WILL spam the log, and try to send messages every 3 seconds! This ugliness is neede to get unit testing to this depth.
def run(self, forever=True): atexit.register(self.stop) + # Clean .sending- lock files from queue + if self.clean_lock_links: + self.maildir._cleanLockLinks() + # Set up logger for mailer + if hasattr(self.mailer, 'set_logger'): + self.mailer.set_logger(self.log)
The interface requires all mailers to have a set_logger(), so why the hasattr test?
Sorry, I have left this!
Overall I like the new features *a lot*. The stylistic issues are mostly minor. The most important of those are
* Fix the integer division bug (easy, but a unit test would be good).
* Resolve the exception name confusion (easy).
* Clearly distinguish queue IDs from RFC-2822 message IDs (should be easy).
All done
And I have a questions:
* What's up with retry_interval? _queueRetryWait doesn't do anything in the inner loop, after sleeping for retry_interval seconds -- it just checks self.__stopped. I suppose this makes Zope 3 shutdown (or restart) faster, but the name seems to be wrong -- there are no actual retries made after retry_interval seconds.
If read code closely and look at state machine diagrams you will notice that .sending- links are removed before entering retry wait, ready for the sending process to restart and retry sending unsent queued messages!
Thank you very much for diving into the murky depths and improving zope.sendmail!
It 's been a pleasure. This review is also improving our code base quality, while getting these much needed changes out there! There is still a fault with zope.sendmail when it comes to sending a single email message to multiple recipients. To do it properly, the send state of the message for each recipient should be tracked. This would necessitate a major rewrite. The code as it is should handle sending such messages to a Smarthost if the Smarthost MTA is configured correctly to receive messages without content checking from Zope 3. I suggest that another mailer using the /usr/sbin/sendmail or /usr/lib/sendmail binaries be used to get around this. I know that Windows does not have this, and that some system administrators will be cheapskates and try direct sending, but that's life! Cheers, Matthew Grant
Matthew Grant wrote:
Just done edits you suggested. Some Unit tests still to be written.
If you wrote the tests first *AS YOU SHOULD* this wouldn't be a problem. Tests written after the case have a tendency to omit edge cases...
Won't be doing one for that /1000 as tehre are more important things to check like exception functionality.
eh? cheers, Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
Chris, FYI - Test coverage in zope.sendmail was originally pretty thin. The work I have done is a big improvement. Zope.sendmail is a corner of Zope3 which has not been implemented that well, and this show up in the terms of its quality and sparse unit test coverage. Patience please! Cheers, Matthew On Tue, 2008-03-25 at 12:50 +0000, Chris Withers wrote:
Matthew Grant wrote:
Just done edits you suggested. Some Unit tests still to be written.
If you wrote the tests first *AS YOU SHOULD* this wouldn't be a problem. Tests written after the case have a tendency to omit edge cases...
Won't be doing one for that /1000 as tehre are more important things to check like exception functionality.
eh?
cheers,
Chris
Matthew Grant wrote:
FYI - Test coverage in zope.sendmail was originally pretty thin.
Hmmm, apply some svn blame, find the culprite and lets arrange some burning at the stake... Seriously, am I right in remembering "Zope 3" and all things related to have been "test first" from day 1?
Zope3 which has not been implemented that well, and this show up in the terms of its quality and sparse unit test coverage.
If it's that bad, bin it and start again... cheers, Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
participants (3)
-
Chris Withers -
Marius Gedminas -
Matthew Grant