On Aug 17, 2009, at 01:48 PM, Данило Шеган wrote: >I've applied most of your recommendations. The ones I didn't were about >'positive tests' (i.e. is 'is not None' really a negative test?) and I >haven't provided unit tests. Still, I've modified doctests to use >mail_helpers, so that resulted in a nice big incremental diff. Indeed, that looks fantastic. Thanks! >У пет, 14. 08 2009. у 16:05 -0400, Barry Warsaw пише: >> > * doc/poexport-request.txt, >> > doc/poexport-request-productseries.txt, >> > doc/poexport-queue.txt: >> > There are a lot of changes to tests that test how emails are generated: >> > basically, half of the branch is there (440 lines). The changes there >> > include introducing a shared helper print_mail_subject_and_body method, >> > print subject along with body of the email, and changes to test new >> > email contents and subjects. Some minor changes to how ExportResult is >> > constructed were necessary as well to conform with the new API (i.e. pass >> > person, requested exports and logger). >> >> Do you know about lp.testing.mail_helpers.print_emails()? I generally use >> that plus ellipses to cut down on the output. Is there a reason why you'd >> want print_mail_subject_and_body instead? > >Yes, I've looked through existing helpers. I've already struggled to >keep the diff size reasonable, and this would have made it only worse. >Though, I am happy to migrate existing tests to the use them, and I did >so. Note that only this change means 640 lines of diff in the >incremental diff ;) That's the way to do it! :) Really, it looks great now. >> === added file 'lib/lp/translations/emailtemplates/poexport-failure-admin-notification.txt' >> --- lib/lp/translations/emailtemplates/poexport-failure-admin-notification.txt 1970-01-01 00:00:00 +0000 >> +++ lib/lp/translations/emailtemplates/poexport-failure-admin-notification.txt 2009-08-14 16:33:57 +0000 >> > @@ -0,0 +1,16 @@ >> > +Hello admins, >> >> Maybe "Hello Launchpad administrators," ? > >Sure, fixed. Note that these emails only ended up on our internal >error-reports list, but no reason not to make them even nicer :) If we can't be nice to ourselves, we can't be nice to our users :) >> > +Automatic message from Launchpad.net — https://launchpad.net/ >> >> Wow, this is like a failure failure. :) > >My mail client linkifies the second, but not the first. I still prefer >the former in an email message, so I made it > > "Automatic message from Launchpad.net." > >in all the email templates. Y'know, I didn't really notice that that was an em-dash. I thought it was a plain old '-' but I guess that was just my editor's font playing tricks on me. In any case, I think keeping it pure ASCII is a good thing (as you saw by removing all those utf-8 conversions). But I wasn't actually complaining about it, I was just observing that the message appeared to be sent when a failure message failed. Still, I'm glad my Friday-addled brain was able to reach through the fog and offer a not-horrible suggestion. ;) >> Is it really more convenient to access the logger through the instance rather >> than use the module global? I can almost promise you that it isn't noticeably >> faster :). > >Considering how tests use MockLogger for whatever reason, I ain't >changing this bit right now either. I fully expect it to be slower, but >there are more important things for us to work on than fix every single >problem in our ancient tests :) Good point! It's not worth changing (right now anyway). >> I almost wish there was some interface and adapter for this. It's probably >> more work, but I'll through it out there in the off chance you think it might >> make the code cleaner. > >It's noticably more work for one single place we'd use it in. As >mentioned before, this is only so we can use the best subject (or well, >much improved one over what the existing case is) in our 'download >available' notification emails. Yep, I was just musing. You're rightly ignoring it. :) >> > + >> > + if productseries is not None: >> > + root = '%s %s' % ( >> > + productseries.product.displayname, >> > + productseries.name) >> > + else: >> >> Please switch this to a positive test, e.g. > >Heh, I'd actually call this a "positive" test, since I am negating a >comparison to None. It made infinitely more sense to me when re-reading >the code, because it's obvious that I can use productseries in the >branch. I can see where you're coming from. In general, it's much more difficult to work out the logic in your head when you use 'not', and my general rule is that the main condition should never use 'not' if you're doing a binary test. For example "if not disabled" is more difficult to suss out than "if enabled". "if thing is None" isn't really a negative test (in my mind) because it has a legitimate positive meaning. So I almost always prefer it because it reduces the number of negations I have to keep in my head. >> > + root = '%s %s %s' % ( >> > + distroseries.distribution.displayname, >> > + distroseries.displayname, >> > + sourcepackagename.name) >> > + if title is not None: >> > + return '%s - %s' % (root, title) >> > + else: >> > + return root >> >> And this too. > >I'd put out the same argument here as above. I.e. I am using `title` >only where I am certain it's not None. Being explicit (instead of >relying on an 'else' of 'is None' check) seems to make more sense to me. Just wait until Python 2.6 when you can write this like so: return (root if title is None else '%s - %s' % (root, title)) :). I won't push this point any more in this case. I've laid out the general principle and I'll leave it for you to decide in this specific case. Maybe I've planted a seed for next time though :). >> > + # Determine productseries or sourcepackage for any >> > + # productseries/sourcepackage an export was requested at. >> > + if potemplate.productseries is not None: >> > + productseries.add(potemplate.productseries) >> > + elif potemplate.sourcepackagename is not None: >> > + sourcepackage = potemplate.distroseries.getSourcePackage( >> > + potemplate.sourcepackagename) >> > + sourcepackages.add(sourcepackage) >> > + else: >> > + pass >> >> Please include a comment before the pass, explaining why that branch is empty. > >I've actually made it an assertion instead. Much better, thanks! > >> > + if IPOTemplate.providedBy(export_requested_at): >> > + if len(sourcepackages) > 0: >> > + sp = sourcepackages.pop() >> > + if sp.getCurrentTranslationTemplates().count() == 1: >> > + export_requested_at = sp >> > + elif len(productseries) > 0: >> > + ps = productseries.pop() >> > + if ps.getCurrentTranslationTemplates().count() == 1: >> > + export_requested_at = ps >> >> I don't much like the abbreviations. What about using 'next' instead? > >Good catch, they are left-overs from when I was prototyping the code in >'make harness'. And, using a variable there (I used 'container') allows >me to simplify the code further (getCurrentTranslationTemplates() call >is identical on either SourcePackage or ProductSeries). Excellent. >> Why doesn't get_email_template() do the decoding to utf-8 automatically? When >> would you ever not want the email messages as a string instead of bytes? > >I have no idea, and I am not willing to fix it in this branch and risk >who knows what failing tests I might run into. :) Instead, I've removed >the decoding since I removed the em-dash I used in my lovely signature >you didn't like: > > "Automatic email from Launchpad.net https://launchpad.net/" > >:) Agreed! >> > + def _getSuccessEmailBody(self): >> > """Send an email notification about the export working.""" >> > - return textwrap.dedent(''' >> >> Same here, DQTQ is probably better (double quoted triple quoted string :). > >I guess you mistakenly haven't noticed that this is the code that is >being removed to be replaced by an external email template :) Y'know, I caught myself doing that a number of times, but this one slipped through the cracks ;). Your incremental diff looks great and I appreciate the cleanups you made along the way. Using print_emails() makes the code much cleaner and more consistent. The branch looks great, and thanks for the changes. r=me