Merge "hacking: Modify checks for translated logs"
This commit is contained in:
Zuul
Gerrit Code Review
commit
d9cd0e1f1c
|
|
@ -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
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue