hacking: Modify checks for translated logs
The N319 check previously asserted that debug-level logs were not translated. Now that we've removed all log translations, we can generalize this to all logs. We reuse the same number since these numbers are really just metadata and not public contracts. This also allows us to update the N323 and N326 checks, which ensure we import the translation function, '_', wherever it's used and don't concatenate translated and non-translated strings. Since we're no longer translating logs and the '_LE', '_LW' and '_LI' symbols are no longer provided, we don't need to consider logs in either of these cases. Change-Id: I64d139ad660bc382e8b9d7c8cd03352b26aadafd Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
This commit is contained in:
Stephen Finucane
parent
21fecc7060
commit
45a88f08b4
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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 = """
|
||||
|
|
|
|||
2
tox.ini
2
tox.ini
|
|
@ -272,7 +272,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
|
||||
|
|
|
|||
Loading…
Reference in New Issue