diff --git a/HACKING.rst b/HACKING.rst index f5083cfbd029..46c17d41a876 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -25,7 +25,7 @@ Nova Specific Commandments assertIsInstance(A, B). - [N317] Change assertEqual(type(A), B) by optimal assert like assertIsInstance(A, B) -- [N319] Validate that debug level logs are not translated. +- [N319] Validate that logs are not translated. - [N320] Setting CONF.* attributes directly in tests is forbidden. Use self.flags(option=value) instead. - [N322] Method's default argument shouldn't be mutable diff --git a/doc/source/reference/i18n.rst b/doc/source/reference/i18n.rst index 799049b91fcb..7795fbe35175 100644 --- a/doc/source/reference/i18n.rst +++ b/doc/source/reference/i18n.rst @@ -15,9 +15,7 @@ network). One upon a time there was an effort to translate log messages in OpenStack projects. But starting with the Ocata release these are no -longer being supported. Log messages **should not** be translated. Any -use of ``_LI()``, ``_LW()``, ``_LE()``, ``_LC()`` are vestigial and -will be removed over time. No new uses of these should be added. +longer being supported. Log messages **should not** be translated. You should use the basic wrapper ``_()`` for strings which are not log messages that are expected to get to an end user:: @@ -25,6 +23,7 @@ messages that are expected to get to an end user:: raise nova.SomeException(_('Invalid service catalogue')) Do not use ``locals()`` for formatting messages because: + 1. It is not as clear as using explicit dicts. 2. It could produce hidden errors during refactoring. 3. Changing the name of a variable causes a change in the message. diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 10f6d8222d27..595ba89f0e7e 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -76,9 +76,7 @@ asse_true_false_with_in_or_not_in_spaces = re.compile(r"assert(True|False)" r"[\[|'|\"](, .*)?\)") asse_raises_regexp = re.compile(r"assertRaisesRegexp\(") conf_attribute_set_re = re.compile(r"CONF\.[a-z0-9_.]+\s*=\s*\w") -translated_log = re.compile( - r"(.)*LOG\.(audit|error|info|critical|exception)" - r"\(\s*_\(\s*('|\")") +translated_log = re.compile(r"(.)*LOG\.\w+\(\s*_\(\s*('|\")") mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])") string_translation = re.compile(r"[^_]*_\(\s*('|\")") underscore_import_check = re.compile(r"(.)*import _(.)*") @@ -302,21 +300,16 @@ def check_python3_xrange(logical_line): @core.flake8ext -def no_translate_debug_logs(logical_line, filename): - """Check for 'LOG.debug(_(' +def no_translate_logs(logical_line, filename): + """Check for 'LOG.foo(_(' - As per our translation policy, - https://wiki.openstack.org/wiki/LoggingStandards#Log_Translation - we shouldn't translate debug level logs. - - * This check assumes that 'LOG' is a logger. - * Use filename so we can start enforcing this in specific folders instead - of needing to do so all at once. + As per our translation policy, we shouldn't translate logs. + This check assumes that 'LOG' is a logger. N319 """ - if logical_line.startswith("LOG.debug(_("): - yield (0, "N319 Don't translate debug level logs") + if translated_log.match(logical_line): + yield (0, "N319 Don't translate logs") @core.flake8ext @@ -371,8 +364,7 @@ def check_explicit_underscore_import(logical_line, filename): elif (underscore_import_check.match(logical_line) or custom_underscore_check.match(logical_line)): UNDERSCORE_IMPORT_FILES.append(filename) - elif (translated_log.match(logical_line) or - string_translation.match(logical_line)): + elif string_translation.match(logical_line): yield (0, "N323: Found use of _() without explicit import of _ !") @@ -474,14 +466,15 @@ class CheckForTransAdd(BaseASTChecker): CHECK_DESC = ('N326 Translated messages cannot be concatenated. ' 'String should be included in translated message.') - TRANS_FUNC = ['_', '_LI', '_LW', '_LE', '_LC'] + TRANS_FUNC = ['_'] 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) + for node_x in (node.left, node.right): + if isinstance(node_x, ast.Call): + if isinstance(node_x.func, ast.Name): + if node_x.func.id == '_': + self.add_error(node_x) super(CheckForTransAdd, self).generic_visit(node) diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index b9f413e52be5..35ffb3008335 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -173,15 +173,15 @@ class HackingTestCase(test.NoDBTestCase): self.assertEqual(len(list(checks.assert_true_or_false_with_in( "self.assertFalse(some in list1 and some2 in list2)"))), 0) - def test_no_translate_debug_logs(self): - self.assertEqual(len(list(checks.no_translate_debug_logs( + def test_no_translate_logs(self): + self.assertEqual(len(list(checks.no_translate_logs( "LOG.debug(_('foo'))", "nova/scheduler/foo.py"))), 1) - self.assertEqual(len(list(checks.no_translate_debug_logs( + self.assertEqual(len(list(checks.no_translate_logs( "LOG.debug('foo')", "nova/scheduler/foo.py"))), 0) - self.assertEqual(len(list(checks.no_translate_debug_logs( - "LOG.info(_('foo'))", "nova/scheduler/foo.py"))), 0) + self.assertEqual(len(list(checks.no_translate_logs( + "LOG.info(_('foo'))", "nova/scheduler/foo.py"))), 1) def test_no_setting_conf_directly_in_tests(self): self.assertEqual(len(list(checks.no_setting_conf_directly_in_tests( @@ -215,23 +215,17 @@ class HackingTestCase(test.NoDBTestCase): "defined, undefined = [], {}")))) def test_check_explicit_underscore_import(self): - self.assertEqual(len(list(checks.check_explicit_underscore_import( - "LOG.info(_('My info message'))", - "cinder/tests/other_files.py"))), 1) self.assertEqual(len(list(checks.check_explicit_underscore_import( "msg = _('My message')", "cinder/tests/other_files.py"))), 1) self.assertEqual(len(list(checks.check_explicit_underscore_import( "from cinder.i18n import _", "cinder/tests/other_files.py"))), 0) - self.assertEqual(len(list(checks.check_explicit_underscore_import( - "LOG.info(_('My info message'))", - "cinder/tests/other_files.py"))), 0) self.assertEqual(len(list(checks.check_explicit_underscore_import( "msg = _('My message')", "cinder/tests/other_files.py"))), 0) self.assertEqual(len(list(checks.check_explicit_underscore_import( - "from cinder.i18n import _, _LW", + "from cinder.i18n import _", "cinder/tests/other_files2.py"))), 0) self.assertEqual(len(list(checks.check_explicit_underscore_import( "msg = _('My message')", @@ -403,23 +397,14 @@ class HackingTestCase(test.NoDBTestCase): _ = 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')] + errors = [(9, 10, 'N326'), (10, 24, 'N326')] self._assert_has_errors(code, checker, expected_errors=errors) code = """ diff --git a/tox.ini b/tox.ini index 5402c7edabe8..2645971ba0f0 100644 --- a/tox.ini +++ b/tox.ini @@ -280,7 +280,7 @@ extension = N316 = checks:assert_true_instance N317 = checks:assert_equal_type N335 = checks:assert_raises_regexp - N319 = checks:no_translate_debug_logs + N319 = checks:no_translate_logs N337 = checks:no_import_translation_in_tests N320 = checks:no_setting_conf_directly_in_tests N322 = checks:no_mutable_default_args