From 4ef6c1d4eac7c20b70042f7c668f38c62cfbbf21 Mon Sep 17 00:00:00 2001 From: James Carey Date: Thu, 21 Aug 2014 03:57:30 +0000 Subject: [PATCH] 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 | 1 + nova/api/metadata/vendordata_json.py | 10 ++++--- nova/exception.py | 2 +- nova/hacking/checks.py | 43 +++++++++++++++++++++++----- nova/tests/compute/test_keypairs.py | 2 +- nova/tests/test_hacking.py | 36 +++++++++++++++++++++++ 6 files changed, 81 insertions(+), 13 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 3995a9ba08..34c79b1e21 100644 --- a/HACKING.rst +++ b/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 ------------------- diff --git a/nova/api/metadata/vendordata_json.py b/nova/api/metadata/vendordata_json.py index ee8a938784..a167d16e56 100644 --- a/nova/api/metadata/vendordata_json.py +++ b/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 diff --git a/nova/exception.py b/nova/exception.py index 05c6ecef9e..c0e233bab7 100644 --- a/nova/exception.py +++ b/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): diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index a22f6318f2..c6e929402e 100644 --- a/nova/hacking/checks.py +++ b/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) diff --git a/nova/tests/compute/test_keypairs.py b/nova/tests/compute/test_keypairs.py index 97f8373620..8c923beb29 100644 --- a/nova/tests/compute/test_keypairs.py +++ b/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): diff --git a/nova/tests/test_hacking.py b/nova/tests/test_hacking.py index 205357a1b1..f42af18472 100644 --- a/nova/tests/test_hacking.py +++ b/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)