Code review comment for lp:~benji/launchpad/bug-388997

Yay! Thanks for fixing this for us. The way you implemented this made me sort of expect SimpleVocabulary to have it built in: lookup ought to fail only if you have not set a default term.

Some review notes:

=== added file 'lib/canonical/launchpad/webapp/tests/test_forgiving_vocabulary.py'
--- lib/canonical/launchpad/webapp/tests/test_forgiving_vocabulary.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_forgiving_vocabulary.py 2010-07-14 10:07:44 +0000
@@ -0,0 +1,28 @@
+# Copyright 2009 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).

Could you update the copyright date?

+ def test_normal_lookup(self):
+ """Test that lookups for proper values succeed."""
+ self.assertIs(self.vocabulary.getTerm('term-1'), self.term_1)
+
+ def test_errant_lookup(self):
+ """Test that lookups for invalid values return the default."""
+ self.assertIs(self.vocabulary.getTerm('does-not-exist'), self.term_2)

It may be a matter of personal style but I find that "Test that" doesn't carry its weight in a test description. An assertive "Lookups for proper values succeed" would do for me. In doctests we like to talk about a "narrative" that, instead of circling around the test code pointing at the things it does, describes the same thing that the code illustrates.

--- lib/lp/translations/browser/hastranslationimports.py 2009-11-20 14:15:34 +0000
+++ lib/lp/translations/browser/hastranslationimports.py 2010-07-14 10:07:44 +0000
@@ -391,10 +392,13 @@

[...]

+ # We use a ForgivingSimpleVocabulary because we don't care if a user
+ # provides an invalid value, if they do we just ignore it and show
+ # them all files.
+ return ForgivingSimpleVocabulary(terms, default_term=all_files)

In the comment, the comma before that second "if" is really a full stop.

--- lib/lp/translations/stories/importqueue/xx-translation-import-queue.txt 2010-04-26 16:00:31 +0000
+++ lib/lp/translations/stories/importqueue/xx-translation-import-queue.txt 2010-07-14 10:07:44 +0000
@@ -449,3 +449,23 @@
   1
   >>> print msgs[0]
   Upload ignored. The tarball you uploaded did not contain...
+
+Bad filter_extension
+~~~~~~~~~~~~~~~~~~~~
+
+Very often robots attempt to request URLs with garbage appended to the end.
+Presumably they scraped these URLs from the web and couldn't quite tell where
+the query string ended and the surrounding text began. Here we'll simulate

In our case, it seems to have happened because someone on IRC closed a parenthesis right after the URL, and an IRC log site linkified the URL with the erroneous parenthesis included. And then I suppose the bots kept coming back because the oops pages they get are not marked as errors and are different every time. Which means they must be interesting and "hot," right?

All small stuff. Go land that baby!

Jeroen