openstack
/
neutron-lib
Code Issues Proposed changes

Enforce log messages not being translated 

This is in line with discussion [1] where translators asked to stop
translating log messages, leaving translations for user visible messages
only.

This patch introduces a new N537 check, and since it is more broad than
N533 while covering all cases of the latter check, N533 is removed.

This patch also cleans up _i18n._L? translation hints since they are
internal and won't be allowed anymore.

[1] http://lists.openstack.org/pipermail/openstack-dev/2017-March/114191.html

Related-Bug: #1674569
Change-Id: I2813587824eac1a996c715b0c4bd36b0c9580d73
This commit is contained in:
Ihar Hrachyshka 2017-03-16 09:52:24 +00:00
parent 24ea6f3529
commit b87ecb3b8c
7 changed files with 71 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
HACKING.rst
View File

@ -17,7 +17,7 @@ Neutron Library Specific Commandments
- [N529] Method's default argument shouldn't be mutable
- [N530] No importing of neutron; should be ignored in neutron itself
- [N532] Validate that LOG.warning is used instead of LOG.warn. The latter is deprecated.
- [N533] Validate that debug level logs are not translated
- [N534] Exception messages should be translated
- [N535] Usage of Python eventlet module not allowed
- [N536] Use assertIsNone/assertIsNotNone rather than assertEqual/assertIs to check None values.
- [N537] Don't translate logs.

12
neutron_lib/_i18n.py
View File

@ -20,15 +20,5 @@ import oslo_i18n
_translators = oslo_i18n.TranslatorFactory(domain='neutron_lib')
# The primary translation function using the well-known name "_"
# The translation function using the well-known name "_"
_ = _translators.primary
# Translators for log levels.
#
# The abbreviated names are meant to reflect the usual use of a short
# name like '_'. The "L" is for "log" and the other letter comes from
# the level.
_LI = _translators.log_info
_LW = _translators.log_warning
_LE = _translators.log_error
_LC = _translators.log_critical

5
neutron_lib/callbacks/manager.py
View File

@ -15,7 +15,6 @@ import collections
from oslo_log import log as logging
from oslo_utils import reflection
from neutron_lib._i18n import _LE
from neutron_lib.callbacks import events
from neutron_lib.callbacks import exceptions
from neutron_lib.db import utils as db_utils
@ -176,8 +175,8 @@ class CallbacksManager(object):
event.startswith(events.PRECOMMIT)
)
if not abortable_event:
LOG.exception(_LE("Error during notification for "
"%(callback)s %(resource)s, %(event)s"),
LOG.exception("Error during notification for "
"%(callback)s %(resource)s, %(event)s",
{'callback': callback_id,
'resource': resource, 'event': event})
else:

2
neutron_lib/hacking/checks.py
View File

@ -257,7 +257,7 @@ def factory(register):
register(check_python3_no_iteritems)
register(no_mutable_default_args)
register(check_neutron_namespace_imports)
register(translation_checks.no_translate_debug_logs)
register(translation_checks.no_translate_logs)
register(translation_checks.check_log_warn_deprecated)
register(translation_checks.check_raised_localized_exceptions)
register(assert_equal_none)

61
neutron_lib/hacking/translation_checks.py
View File

@ -15,14 +15,22 @@
import re
_all_log_levels = {'critical', 'error', 'exception', 'info', 'warning'}
_all_hints = {'_LC', '_LE', '_LI', '_', '_LW'}
_log_warn = re.compile(
r"(.)*LOG\.(warn)\(\s*('|\"|_)")
r"(.)*LOG\.(warn)\(\s*('|\")")
def _translation_is_not_expected(filename):
_log_translation_hint = re.compile(
r".*LOG\.(%(levels)s)\(\s*(%(hints)s)\(" % {
'levels': '|'.join(_all_log_levels),
'hints': '|'.join(_all_hints),
})
def _translation_checks_not_enforced(filename):
# Do not do these validations on tests
return any(pat in filename for pat in ["/tests/", "rally-jobs/plugins/"])
@ -41,28 +49,6 @@ def check_log_warn_deprecated(logical_line, filename):
yield (0, msg)
def no_translate_debug_logs(logical_line, filename):
"""N533 - Don't translate debug level logs.
Check for 'LOG.debug(_(' and 'LOG.debug(_Lx('
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.
:param logical_line: The logical line to check.
:param filename: The file name where the logical line exists.
:returns: None if the logical line passes the check, otherwise a tuple
is yielded that contains the offending index in logical line and a
message describe the check validation failure.
"""
for hint in _all_hints:
if logical_line.startswith("LOG.debug(%s(" % hint):
yield(0, "N533 Don't translate debug level logs")
def check_raised_localized_exceptions(logical_line, filename):
"""N534 - Untranslated exception message.
@ -72,7 +58,7 @@ def check_raised_localized_exceptions(logical_line, filename):
is yielded that contains the offending index in logical line and a
message describe the check validation failure.
"""
if _translation_is_not_expected(filename):
if _translation_checks_not_enforced(filename):
return
logical_line = logical_line.strip()
@ -83,3 +69,28 @@ def check_raised_localized_exceptions(logical_line, filename):
if exception_msg.startswith("\"") or exception_msg.startswith("\'"):
msg = "N534: Untranslated exception message."
yield (logical_line.index(exception_msg), msg)
def no_translate_logs(logical_line, filename):
"""N537 - Don't translate logs.
Check for 'LOG.*(_(' and 'LOG.*(_Lx('
Translators don't provide translations for log messages, and operators
asked not to translate them.
* This check assumes that 'LOG' is a logger.
:param logical_line: The logical line to check.
:param filename: The file name where the logical line exists.
:returns: None if the logical line passes the check, otherwise a tuple
is yielded that contains the offending index in logical line and a
message describe the check validation failure.
"""
if _translation_checks_not_enforced(filename):
return
msg = "N537: Log messages should not be translated!"
match = _log_translation_hint.match(logical_line)
if match:
yield (logical_line.index(match.group()), msg)

23
neutron_lib/tests/unit/hacking/test_checks.py
View File

@ -134,15 +134,24 @@ class HackingTestCase(base.BaseTestCase):
self.assertLineFails(f, 'from neutron import context')
self.assertLineFails(f, 'import neutron.common.config')
def test_no_translate_debug_logs(self):
for hint in tc._all_hints:
bad = "LOG.debug(%s('bad'))" % hint
self.assertEqual(
1, len(list(tc.no_translate_debug_logs(bad, 'f'))))
def test_no_log_translations(self):
for log in tc._all_log_levels:
for hint in tc._all_hints:
bad = 'LOG.%s(%s("Bad"))' % (log, hint)
self.assertEqual(
1, len(list(tc.no_translate_logs(bad, 'f'))))
# Catch abuses when used with a variable and not a literal
bad = 'LOG.%s(%s(msg))' % (log, hint)
self.assertEqual(
1, len(list(tc.no_translate_logs(bad, 'f'))))
# Do not do validations in tests
ok = 'LOG.%s(_("OK - unit tests"))' % log
self.assertEqual(
0, len(list(tc.no_translate_logs(ok, 'f/tests/f'))))
def test_check_log_warn_deprecated(self):
bad = "LOG.warn(_LW('i am deprecated!'))"
good = "LOG.warning(_LW('zlatan is the best'))"
bad = "LOG.warn('i am deprecated!')"
good = "LOG.warning('zlatan is the best')"
f = tc.check_log_warn_deprecated
self.assertLineFails(f, bad, '')
self.assertLinePasses(f, good, '')

14
releasenotes/notes/new-hacking-check-no-log-translations-4a430a38aeb06452.yaml Normal file
View File

@ -0,0 +1,14 @@
---
features:
- |
New ``N537`` hacking check is introduced that enforces no logging message
translations, in any logging level. The check is enabled by default. Also,
the ``N533`` hacking check is now removed because it is covered by
``N537``.
upgrade:
- |
Library consumers may need to adopt their code to new requirements of ``N537`` hacking check,
removing translation markers from all logging messages. If for some reason
it doesn't fit the project, consumers can disable the new hacking check
using ``ignore`` statement in ``flake8`` section of their ``tox.ini`` file,
or by other means.