Remove concatenation with translated messages

Translated messages should not be expanded by concatenation.  Instead
the additional text should be replacement text or part of the translated
message to allow the translators as much information as possible when
doing the translations.

Also, when lazy translation is enabled, the object returned does not
support concatenation and raises an exception.  Thus this patch is
needed to support blueprint: i18n-enablement.

This patch includes a hacking check for concatenation with a translated
message.

Change-Id: I8b368d275faa14b4750735445321874ce1da37d5
Partially-Implements: blueprint i18n-enablement

HACKING.rst

@@ -38,6 +38,7 @@ Nova Specific Commandments
 - [N323] Ensure that the _() function is explicitly imported to ensure proper translations.
 - [N324] Ensure that jsonutils.%(fun)s must be used instead of json.%(fun)s
 - [N325] str() cannot be used on an exception.  Remove use or use six.text_type()
+- [N326] Translated messages cannot be concatenated.  String should be included in translated message.
 
 Creating Unit Tests
 -------------------

nova/api/metadata/vendordata_json.py

@@ -44,13 +44,15 @@ class JsonFileVendorData(base.VendorDataDriver):
                     data = jsonutils.load(fp)
             except IOError as e:
                 if e.errno == errno.ENOENT:
-                    LOG.warn(logprefix + _LW("file does not exist"))
+                    LOG.warn(_LW("%(logprefix)sfile does not exist"),
+                             {'logprefix': logprefix})
                 else:
-                    LOG.warn(logprefix + _LW("Unexpected IOError when "
-                                             "reading"))
+                    LOG.warn(_LW("%(logprefix)unexpected IOError when "
+                                 "reading"), {'logprefix': logprefix})
                 raise e
             except ValueError:
-                LOG.warn(logprefix + _LW("failed to load json"))
+                LOG.warn(_LW("%(logprefix)sfailed to load json"),
+                         {'logprefix': logprefix})
                 raise
 
         self._data = data

nova/exception.py

@@ -292,7 +292,7 @@ class InvalidVolume(Invalid):
 
 
 class InvalidVolumeAccessMode(Invalid):
-    msg_fmt = _("Invalid volume access mode") + ": %(access_mode)s"
+    msg_fmt = _("Invalid volume access mode: %(access_mode)s")
 
 
 class InvalidMetadata(Invalid):

nova/hacking/checks.py

@@ -102,6 +102,13 @@ class BaseASTChecker(ast.NodeVisitor):
         error = (node.lineno, node.col_offset, message, self.__class__)
         self._errors.append(error)
 
+    def _check_call_names(self, call_node, names):
+        if isinstance(call_node, ast.Call):
+            if isinstance(call_node.func, ast.Name):
+                if call_node.func.id in names:
+                    return True
+        return False
+
 
 def import_no_db_in_virt(logical_line, filename):
     """Check for db calls from nova/virt
@@ -367,16 +374,37 @@ class CheckForStrExc(BaseASTChecker):
                 super(CheckForStrExc, self).generic_visit(node)
 
     def visit_Call(self, node):
-        if isinstance(node.func, ast.Name):
-            if node.func.id == 'str':
-                if node not in self.already_checked:
-                    self.already_checked.append(node)
-                    if isinstance(node.args[0], ast.Name):
-                        if node.args[0].id in self.name:
-                            self.add_error(node.args[0])
+        if self._check_call_names(node, ['str']):
+            if node not in self.already_checked:
+                self.already_checked.append(node)
+                if isinstance(node.args[0], ast.Name):
+                    if node.args[0].id in self.name:
+                        self.add_error(node.args[0])
         super(CheckForStrExc, self).generic_visit(node)
 
 
+class CheckForTransAdd(BaseASTChecker):
+    """Checks for the use of concatenation on a translated string.
+
+    Translations should not be concatenated with other strings, but
+    should instead include the string being added to the translated
+    string to give the translators the most information.
+    """
+
+    CHECK_DESC = ('N326 Translated messages cannot be concatenated.  '
+                  'String should be included in translated message.')
+
+    TRANS_FUNC = ['_', '_LI', '_LW', '_LE', '_LC']
+
+    def visit_BinOp(self, node):
+        if isinstance(node.op, ast.Add):
+            if self._check_call_names(node.left, self.TRANS_FUNC):
+                self.add_error(node.left)
+            elif self._check_call_names(node.right, self.TRANS_FUNC):
+                self.add_error(node.right)
+        super(CheckForTransAdd, self).generic_visit(node)
+
+
 def factory(register):
     register(import_no_db_in_virt)
     register(no_db_session_in_public_api)
@@ -396,3 +424,4 @@ def factory(register):
     register(check_explicit_underscore_import)
     register(use_jsonutils)
     register(CheckForStrExc)
+    register(CheckForTransAdd)

nova/tests/compute/test_keypairs.py

@@ -120,7 +120,7 @@ class CreateImportSharedTestMixIn(object):
         self.assertEqual(expected_message, unicode(exc))
 
     def assertInvalidKeypair(self, expected_message, name):
-        msg = _('Keypair data is invalid') + ': ' + expected_message
+        msg = _('Keypair data is invalid: %s') % expected_message
         self.assertKeyNameRaises(exception.InvalidKeypair, msg, name)
 
     def test_name_too_short(self):

nova/tests/test_hacking.py

@@ -301,3 +301,39 @@ class HackingTestCase(test.NoDBTestCase):
                """
         errors = [(8, 20, 'N325'), (8, 29, 'N325')]
         self._assert_has_errors(code, checker, expected_errors=errors)
+
+    def test_trans_add(self):
+
+        checker = checks.CheckForTransAdd
+        code = """
+               def fake_tran(msg):
+                   return msg
+
+
+               _ = fake_tran
+               _LI = _
+               _LW = _
+               _LE = _
+               _LC = _
+
+
+               def f(a, b):
+                   msg = _('test') + 'add me'
+                   msg = _LI('test') + 'add me'
+                   msg = _LW('test') + 'add me'
+                   msg = _LE('test') + 'add me'
+                   msg = _LC('test') + 'add me'
+                   msg = 'add to me' + _('test')
+                   return msg
+               """
+        errors = [(13, 10, 'N326'), (14, 10, 'N326'), (15, 10, 'N326'),
+                  (16, 10, 'N326'), (17, 10, 'N326'), (18, 24, 'N326')]
+        self._assert_has_errors(code, checker, expected_errors=errors)
+
+        code = """
+               def f(a, b):
+                   msg = 'test' + 'add me'
+                   return msg
+               """
+        errors = []
+        self._assert_has_errors(code, checker, expected_errors=errors)