- Zope-Dev - Zope lists

Zope Tests: 5 OK
by Zope Tests Summarizer 29 Mar '08

29 Mar '08

Summary of messages to the zope-tests list. Period Fri Mar 28 12:00:00 2008 UTC to Sat Mar 29 12:00:00 2008 UTC. There were 5 messages: 5 from Zope Tests. Tests passed OK --------------- Subject: OK : Zope-2.8 Python-2.3.6 : Linux From: Zope Tests Date: Fri Mar 28 21:50:54 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009320.html Subject: OK : Zope-2.9 Python-2.4.4 : Linux From: Zope Tests Date: Fri Mar 28 21:52:24 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009321.html Subject: OK : Zope-2.10 Python-2.4.4 : Linux From: Zope Tests Date: Fri Mar 28 21:53:54 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009322.html Subject: OK : Zope-2.11 Python-2.4.4 : Linux From: Zope Tests Date: Fri Mar 28 21:55:25 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009323.html Subject: OK : Zope-trunk Python-2.4.4 : Linux From: Zope Tests Date: Fri Mar 28 21:56:55 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009324.html

1 0

Re: AW: SVN: z3c.zrtresource/trunk/src/z3c/zrtresource/zrtresource.pyz3c.zrtresource is made to work in Zope2. A docstring wasadded as the zope2 publisher won't publish otherwise. Added aconditional import on Five's FileResource. Only oneconditiona
by Tres Seaver 28 Mar '08

28 Mar '08

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Wichert Akkerman wrote: > Previously Nikolay Kim wrote: >>>> Probably this is not much important enough for that >>>> *import* usecase, but if we start with that, can you tell >>>> me wheres the border with mixin code into Zope3 packages >>>> and what is Ok and where do we stop? >>> I agree it should rarely be used, but it was just one import so then >>> I thought it was ok. >> Conditional import are ugly and many of us doesn't use zope2 at all. > > And many of us do use Zope2 and want to be able to use all the nice > Zope3 technology. A single conditional import is an extremely small > price to pay to give a large group of people access to new technology. > > I thought the componentized Zope 3 was all about making it easier to > reuse modules. Did that change? I don't know. I think the objection to the change was that it introduced a dependency inversion, as well as making it impossible to test the code properly (you would need to run the tests in both an environment with and without Five present). In general, bridging gaps between the "depended on" Zope 3 stuff and the "dependent" Zope2 stuff is not supposed to involve introducing ugliness into the Zope 3 stuff: the Five package has been the "ugly magnet" since it was first created, for exactly this reason. (Followups reset to zope-dev: the checkins list isn't supposed to be fore discussion). Tres. - -- =================================================================== Tres Seaver +1 540-429-0999 tseaver(a)palladion.com Palladion Software "Excellence by Design" http://palladion.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFH7Wyk+gerLs4ltQ4RAugcAJ49/v6V+4aIZvVJBHeHfON4JbVLnQCg3Vfq 6EKI3wvuVOzYTPwjQV6df+Y= =P4UJ -----END PGP SIGNATURE-----

1 0

Zope Tests: 5 OK
by Zope Tests Summarizer 28 Mar '08

28 Mar '08

Summary of messages to the zope-tests list. Period Thu Mar 27 12:00:00 2008 UTC to Fri Mar 28 12:00:00 2008 UTC. There were 5 messages: 5 from Zope Tests. Tests passed OK --------------- Subject: OK : Zope-2.8 Python-2.3.6 : Linux From: Zope Tests Date: Thu Mar 27 21:50:29 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009315.html Subject: OK : Zope-2.9 Python-2.4.4 : Linux From: Zope Tests Date: Thu Mar 27 21:51:59 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009316.html Subject: OK : Zope-2.10 Python-2.4.4 : Linux From: Zope Tests Date: Thu Mar 27 21:53:29 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009317.html Subject: OK : Zope-2.11 Python-2.4.4 : Linux From: Zope Tests Date: Thu Mar 27 21:55:00 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009318.html Subject: OK : Zope-trunk Python-2.4.4 : Linux From: Zope Tests Date: Thu Mar 27 21:56:30 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009319.html

1 0

[Zope 2.12] Eggification of Zope 2 - pros and cons?
by Andreas Jung 28 Mar '08

28 Mar '08

Hi, during the latest 'zope.publisher' thread on zope-dev I came up with the proposal to eggify the Zope core for the Zope 2.12 release. I would like to start a discussion about the pros and cons, risks and advantages of any eggification effort. Chris favors a 'big' Zope egg with some dependencies (like ZODB) stripped out. I would prefer a more broader approach. One of the reasons are company-internals modifications to the Zope core. Right now we maintain a more or less heavy modified version of Zope 2.8 in our repos (making Zope upgrades pretty hard). A better modularization would help us here. Another example: the Plone people maintained a Zope 2.10 branch with experimental ZODB blob support. With an eggified version of ZODB you could easily switch the eggs (easily spoken). So before promoting the eggification as an ultimate goal, let's discuss what we really need and want. A complete eggification just for the sake of eggs is possibly not the goal :-) Andreas

6 10

Re: SVN: z3c.schema2xml/branches/jw-experiment/ Fairly mechanical changes where IXMLGenerators are now multi
by Tres Seaver 27 Mar '08

27 Mar '08

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jan-Wijbrand Kolman wrote: > Log message for revision 84963: > Fairly mechanical changes where IXMLGenerators are now multi > adapters on field and request (essentially making them into views). Why would I need or want a request to do XML serialization? If you have such a use case, can you make the request-dependent bits separate from the existing adapters, so that applications which use them in situations where there is no request in sight continue to work? Tres. - -- =================================================================== Tres Seaver +1 540-429-0999 tseaver(a)palladion.com Palladion Software "Excellence by Design" http://palladion.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFH6/Sj+gerLs4ltQ4RAvA1AKDR0ImgPF5F/GxM/lrB2ma84efjwQCeIOy8 Bb+NqUVk+02n7WLqJI9z3ig= =6q2A -----END PGP SIGNATURE-----

1 0

Packaging Zope for Fedora
by Timothy Selivanow 27 Mar '08

27 Mar '08

As the subject implies, I'm working on a package for Fedora. I've come across some missing dependencies, and some that are different versions. Because of the way that Python packages are put together (shared library environment), I'm trying to strip out everything that is not Zope specific (e.g. mechanize, docutils, etc.) that way Zope won't clobber already existing packages, and new packages won't clobber Zope. Here is my list (that I've discovered so far...) and some notes and questions. I would appreciate any information on resolving these. My list was taken from Zope-3.4.0c1/build/lib.linux-x86_64-2.5 (in my arch specific case...) after doing a `python install.py build` (side note, why is it called install.py and not setup.py?). BTrees -- Does not currently exist in Fedora as a separate package. Is this of Zope origin? (If not, where? Required version?) If Zope origin, is it useful outside of Zope? ClientForm -- This currently exists in Fedora (version 0.2.7) Diffing the directories and files reveals differences. What version is required for Zope? What about newer versions? twisted -- This currently exists in Fedora (version 2.4.0) Diffing the directories reveals that is it mostly compatible, however, missing some features (I have a list). What parts are required by Zope? Some of the missing parts are no longer being maintained, or are incompatible with newest Twisted (2.5.0). Thoughts? mechanize -- This currently exists in Fedora (version 0.1.6) Diffing the directories and files reveals differences. What version is required for Zope? What about newer versions? persistent -- Does not currently exist in Fedora as a separate package. Is this of Zope origin? (If not, where? Required version?) If Zope origin, is it useful outside of Zope? pytz -- This currently exists in Fedora (version 2006p) Diffing the directories and files reveals differences. What version is required for Zope? What about newer versions? The Zope versions do not have the .py extension. Possible problems with Zope if they do (i.e., lib resolving)? RestrictedPython -- Does not currently exist in Fedora as a separate package. Is this it? <http://pypi.python.org/pypi/RestrictedPython/3.4.2> Required version for Zope? What about newer versions? ThreadedAsync -- Does not currently exist in Fedora as a separate package. Is this of Zope origin? (If not, where? Required version?) If Zope origin, is it useful outside of Zope? transaction -- Does not currently exist in Fedora as a separate package. Is this of Zope origin? (If not, where? Required version?) If not Zope origin, is it useful outside of Zope? Also, there is an existing package in Fedora called python-zope-interface, version 3.0.1 I assume (in F8), and I'm wondering if it would be worth keeping separate (or at least available, updated, maybe with a Conflicts: zope) in Fedora and are there other parts of Zope (e.g. ZODB, ZEO) that would be useful by themselves? I'm sorry if this seems daunting (it is on my side ;) and/or demanding, but I would like this packaged the best possible way for both projects. Who knows, this might show up in future RHEL versions (maybe I'm delusional ;) Thank you for your time and patience. --Tim _________________________________________________________________________ / According to Kentucky state law, every person must take a bath at least \ \ once a year. / ------------------------------------------------------------------------- \ \ \ \ /\ ( ) .( o ).

7 11

Zope Tests: 5 OK
by Zope Tests Summarizer 27 Mar '08

27 Mar '08

Summary of messages to the zope-tests list. Period Wed Mar 26 12:00:00 2008 UTC to Thu Mar 27 12:00:00 2008 UTC. There were 5 messages: 5 from Zope Tests. Tests passed OK --------------- Subject: OK : Zope-2.8 Python-2.3.6 : Linux From: Zope Tests Date: Wed Mar 26 21:52:24 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009310.html Subject: OK : Zope-2.9 Python-2.4.4 : Linux From: Zope Tests Date: Wed Mar 26 21:53:55 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009311.html Subject: OK : Zope-2.10 Python-2.4.4 : Linux From: Zope Tests Date: Wed Mar 26 21:55:25 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009312.html Subject: OK : Zope-2.11 Python-2.4.4 : Linux From: Zope Tests Date: Wed Mar 26 21:56:55 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009313.html Subject: OK : Zope-trunk Python-2.4.4 : Linux From: Zope Tests Date: Wed Mar 26 21:58:25 EDT 2008 URL: http://mail.zope.org/pipermail/zope-tests/2008-March/009314.html

1 0

zope.component.zcml _protectedFactory not protecting?
by Christian Zagrodnick 27 Mar '08

27 Mar '08

Hi, I'm a little confused by the "permission" attribute on the <adapter> statement. First of all, a principal not having the set permission still gets the adapter. That wouldn't be much of a problem if the adapter was securiy-proxied. The adapter is created with the _protectedFactory: def _protectedFactory(original_factory, checker): # This has to be named 'factory', aparently, so as not to confuse # apidoc :( def factory(*args): ob = original_factory(*args) try: ob.__Security_checker__ = checker except AttributeError: ob = Proxy(ob, checker) return ob factory.factory = original_factory return factory I wonder why the factory only creates a security proxy when it cannot assign __Security_checker__ to the adapter. I suppose this is intentional? Regards, -- Christian Zagrodnick gocept gmbh & co. kg · forsterstrasse 29 · 06112 halle/saale www.gocept.com · fon. +49 345 12298894 · fax. +49 345 12298891

1 0

zope.sendmail grantma-retryfixes branch review
by Marius Gedminas 26 Mar '08

26 Mar '08

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&vi… 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&vi… > Index: src/zope/sendmail/configure.zcml > =================================================================== > --- src/zope/sendmail/configure.zcml (revision 84550) > +++ src/zope/sendmail/configure.zcml (revision 84551) > @@ -14,11 +14,18 @@ > >  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(a)example.com' > - toaddrs = ('you(a)example.com', 'him(a)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(a)example.com\n' > + 'X-Zope-To: bar(a)example.com, baz(a)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.

3 7

Reducing dependencies of zope.publisher
by Jim Fulton 26 Mar '08

26 Mar '08

I'd like reduce the barrier of entry for using the Zope 3 zope publisher. Right now, it has quite a few dependencies setuptools, zope.component, zope.event, zope.exceptions, zope.i18n, zope.interface, zope.location, zope.proxy, zope.security, zope.deprecation, and zope.deferredimport. This seems a bit extreme. I can get rid of zope.deprecation and zope.deferredimport directly. I'd like to get rid of more. I'm thinking of creating a separate package that contains much of the guts of zope.publisher, but without most or all of the existing dependencies. I'd then modify zope.publisher to import code from this other package, adding the bits that exist now that cause the dependencies. This new package would a number of things, including skin support, xmlrpc support, mapply, interface declarations and various view base classes that now live in zope.publisher. Thoughts? Objections? Jim -- Jim Fulton Zope Corporation

10 25