From b87ecb3b8cd66cf91d33852cb9c377efa88ba1b5 Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Thu, 16 Mar 2017 09:52:24 +0000 Subject: [PATCH] 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 --- HACKING.rst | 2 +- neutron_lib/_i18n.py | 12 +--- neutron_lib/callbacks/manager.py | 5 +- neutron_lib/hacking/checks.py | 2 +- neutron_lib/hacking/translation_checks.py | 61 +++++++++++-------- neutron_lib/tests/unit/hacking/test_checks.py | 23 ++++--- ...-no-log-translations-4a430a38aeb06452.yaml | 14 +++++ 7 files changed, 71 insertions(+), 48 deletions(-) create mode 100644 releasenotes/notes/new-hacking-check-no-log-translations-4a430a38aeb06452.yaml diff --git a/HACKING.rst b/HACKING.rst index 8a38f04..a662ff1 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -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. diff --git a/neutron_lib/_i18n.py b/neutron_lib/_i18n.py index 90cb59a..d01ac3c 100644 --- a/neutron_lib/_i18n.py +++ b/neutron_lib/_i18n.py @@ -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 diff --git a/neutron_lib/callbacks/manager.py b/neutron_lib/callbacks/manager.py index e693592..ee06bcc 100644 --- a/neutron_lib/callbacks/manager.py +++ b/neutron_lib/callbacks/manager.py @@ -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: diff --git a/neutron_lib/hacking/checks.py b/neutron_lib/hacking/checks.py index fa54769..0582b1b 100644 --- a/neutron_lib/hacking/checks.py +++ b/neutron_lib/hacking/checks.py @@ -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) diff --git a/neutron_lib/hacking/translation_checks.py b/neutron_lib/hacking/translation_checks.py index 2478030..e90694e 100644 --- a/neutron_lib/hacking/translation_checks.py +++ b/neutron_lib/hacking/translation_checks.py @@ -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) diff --git a/neutron_lib/tests/unit/hacking/test_checks.py b/neutron_lib/tests/unit/hacking/test_checks.py index c140b50..0052f55 100644 --- a/neutron_lib/tests/unit/hacking/test_checks.py +++ b/neutron_lib/tests/unit/hacking/test_checks.py @@ -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, '') diff --git a/releasenotes/notes/new-hacking-check-no-log-translations-4a430a38aeb06452.yaml b/releasenotes/notes/new-hacking-check-no-log-translations-4a430a38aeb06452.yaml new file mode 100644 index 0000000..380ac1d --- /dev/null +++ b/releasenotes/notes/new-hacking-check-no-log-translations-4a430a38aeb06452.yaml @@ -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.