Merge lp:~jtv/launchpad/recife-test_translatablemessage-cleanup into lp:~launchpad/launchpad/recife

[Jeroen T. Vermeulen]

test_translatablemessage pre-cleanup

= Pre-cleanup of test_translatablemessage =

This is a preparation for unrelated changes to the same file in work on our Recife feature branch. I'm cleaning up several things in that test:

1. Base class used by all test cases is not a test case, yet inherits from TestCaseWithFactory. Make only the real test cases inherit from that.

2. Useless "test suite" boilerplate at the end of the test. Not needed nowadays. Also takes an import with it.

3. Bad indent. Our coding guidelines forbid method calls in the style of

    foo.goDoSomething(bar, splat,
                      cough, ghee)

To test, run just this one test:
{{{
./bin/test -vvc lp.translations.tests.test_translatablemessage
}}}

No Q/A required, no lint found.

Jeroen

[Данило Шеган (danilo)]

I find rule 3 very weird. One of the suggested styles for function *definitions* is that one, and you are even using that style for class definition where you inherit multiple classes. That smells of inconsistency in our style guide and should probably be fixed. We recently discussed (in the reviewers meeting) changing function definition style to not allow this style either, but I think the consensus was that we don't want to disallow it. Perhaps we should raise it in the reviewers meeting again to change *function call* style instead.

All good otherwise, thanks for the clean-up.

review: Approve (code)

[Данило Шеган (danilo)]

I mean, all is good (not "otherwise"), since the style you are using is definitely acceptable.

[Jeroen T. Vermeulen (jtv)]

Yes, it's weird. I never liked this style for definitions either. I used to indent like that for my first decade or so of C/C++ programming, but found that I like the fixed-indent style better.

I looked for possible justifications for having definitions and calls different, and found only these:

1. For a definition, indenting the continuation with a single tab brings you to the same level as the actual contents of the definition.

There's other ways of dealing with that though; a syntax-coloured docstring will provide a clear distinction for instance.

2. Re-indentation work when editing (grrr) is less likely to get out of hand in a definition than in calls.

That's only an argument of risk limitation though; it doesn't say that one is more or less likely to be more work than the other in the average case.

I wouldn't bother too much with changing the review rules here though, as per my rule of thumb that it's probably a waste of time to impose new rules unless we're also going back to fix the bulk of existing practice. What's already in the file is a much stronger and earlier influence than review rules.

Jeroen

=== modified file 'lib/lp/translations/tests/test_translatablemessage.py'
--- lib/lp/translations/tests/test_translatablemessage.py	2010-11-04 09:32:28 +0000
+++ lib/lp/translations/tests/test_translatablemessage.py	2010-11-16 06:27:02 +0000
@@ -9,7 +9,6 @@
     datetime,
     timedelta,
     )
-from unittest import TestLoader
 
 import pytz
 import transaction
@@ -22,7 +21,7 @@
 from lp.translations.model.translatablemessage import TranslatableMessage
 
 
-class TestTranslatableMessageBase(TestCaseWithFactory):
+class TestTranslatableMessageBase:
     """Common setup for `TranslatableMessage`."""
 
     layer = ZopelessDatabaseLayer
@@ -64,7 +63,8 @@
                 date_updated=date_updated)
 
 
-class TestTranslatableMessage(TestTranslatableMessageBase):
+class TestTranslatableMessage(TestTranslatableMessageBase,
+                              TestCaseWithFactory):
     """Test of `TranslatableMessage` properties and methods."""
 
     def test_sequence(self):
@@ -137,7 +137,8 @@
         self.assertEqual(translation, shared)
 
 
-class TestTranslatableMessageExternal(TestTranslatableMessageBase):
+class TestTranslatableMessageExternal(TestTranslatableMessageBase,
+                                      TestCaseWithFactory):
     """Test of `TranslatableMessage` methods for external translations."""
 
     def setUp(self):
@@ -177,7 +178,8 @@
         self.assertContentEqual([self.external_suggestion], externals)
 
 
-class TestTranslatableMessageSuggestions(TestTranslatableMessageBase):
+class TestTranslatableMessageSuggestions(TestTranslatableMessageBase,
+                                         TestCaseWithFactory):
     """Test of `TranslatableMessage` methods for getting suggestions."""
 
     def gen_now(self):
@@ -198,8 +200,8 @@
     def test_getAllSuggestions(self):
         # There are three different methods to return.
         suggestions = self.message.getAllSuggestions()
-        self.assertContentEqual([self.suggestion1, self.suggestion2],
-                                suggestions)
+        self.assertContentEqual(
+            [self.suggestion1, self.suggestion2], suggestions)
 
     def test_getDismissedSuggestions(self):
         # There are three different methods to return.
@@ -215,11 +217,6 @@
         # Add a suggestion that is newer than the current translation and
         # dismiss it. Also show that getSuggestions only returns translations
         # that are newer than the current one unless only_new is set to False.
-
         self.message.dismissAllSuggestions(self.potemplate.owner, self.now())
         suggestions = self.message.getUnreviewedSuggestions()
         self.assertContentEqual([], suggestions)
-
-
-def test_suite():
-    return TestLoader().loadTestsFromName(__name__)