From 3099cd80500ad0e97ab954c99bbe83211432e595 Mon Sep 17 00:00:00 2001 From: Jeremy Liu Date: Wed, 29 Mar 2017 18:11:44 +0800 Subject: [PATCH] Remove log translation related check Since we're preparing to remove log translation from project, we should remove translation check to ensure tests pass. For log translation removal, see link [1][2]. [1] http://lists.openstack.org/pipermail/openstack-i18n/2016-November/002574.html [2] http://lists.openstack.org/pipermail/openstack-dev/2017-March/113365.html Change-Id: I7d3bd59cf95d9e10c2d578b63dc60313f1d09282 --- HACKING.rst | 29 ------------ barbican/hacking/checks.py | 86 ---------------------------------- barbican/tests/test_hacking.py | 55 ---------------------- 3 files changed, 170 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 2327bafce..028446215 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -12,11 +12,7 @@ Barbican Specific Commandments - [B310] Check for improper use of logging format arguments. - [B311] Use assertIsNone(...) instead of assertEqual(None, ...). - [B312] Use assertTrue(...) rather than assertEqual(True, ...). -- [B313] Validate that debug level logs are not translated. - [B314] str() and unicode() cannot be used on an exception. Remove or use six.text_type(). -- [B315] Translated messages cannot be concatenated. String should be - included in translated message. -- [B316] Log messages, except debug ones, require translations! - [B317] 'oslo_' should be used instead of 'oslo.' - [B318] Must use a dict comprehension instead of a dict constructor with a sequence of key-value pairs. @@ -25,31 +21,6 @@ Barbican Specific Commandments - [B321] Use assertIsNotNone(...) rather than assertNotEqual(None, ...) or assertIsNot(None, ...). -LOG Translations ----------------- - -LOG.debug messages will not get translated. Use ``_LI()`` for -``LOG.info``, ``_LW`` for ``LOG.warning``, ``_LE`` for ``LOG.error`` -and ``LOG.exception``, and ``_LC()`` for ``LOG.critical``. - -``_()`` is preferred for any user facing message, even if it is also -going to a log file. This ensures that the translated version of the -message will be available to the user. - -The log marker functions (``_LI()``, ``_LW()``, ``_LE()``, and ``_LC()``) -must only be used when the message is only sent directly to the log. -Anytime that the message will be passed outside of the current context -(for example as part of an exception) the ``_()`` marker function -must be used. - -A common pattern is to define a single message object and use it more -than once, for the log call and the exception. In that case, ``_()`` -must be used because the message is going to appear in an exception that -may be presented to the user. - -For more details about translations, see -http://docs.openstack.org/developer/oslo.i18n/guidelines.html - Creating Unit Tests ------------------- For every new feature, unit tests should be created that both test and diff --git a/barbican/hacking/checks.py b/barbican/hacking/checks.py index 31a7f20b4..904ce1999 100644 --- a/barbican/hacking/checks.py +++ b/barbican/hacking/checks.py @@ -34,26 +34,6 @@ Guidelines for writing new hacking checks """ -UNDERSCORE_IMPORT_FILES = [] - -log_translation = re.compile( - r"(.)*LOG\.(audit|error|info|critical|exception)\(\s*('|\")") -log_translation_LC = re.compile( - r"(.)*LOG\.(critical)\(\s*(_\(|'|\")") -log_translation_LE = re.compile( - r"(.)*LOG\.(error|exception)\(\s*(_\(|'|\")") -log_translation_LI = re.compile( - r"(.)*LOG\.(info)\(\s*(_\(|'|\")") -log_translation_LW = re.compile( - r"(.)*LOG\.(warning|warn)\(\s*(_\(|'|\")") -translated_log = re.compile( - r"(.)*LOG\.(audit|error|info|warn|warning|critical|exception)" - "\(\s*_\(\s*('|\")") -string_translation = re.compile(r"[^_]*_\(\s*('|\")") -underscore_import_check = re.compile(r"(.)*import _$") -underscore_import_check_multi = re.compile(r"(.)*import (.)*_, (.)*") -# We need this for cases where they have created their own _ function. -custom_underscore_check = re.compile(r"(.)*_\s*=\s*(.)*") oslo_namespace_imports = re.compile(r"from[\s]*oslo[.](.*)") dict_constructor_with_list_copy_re = re.compile(r".*\bdict\((\[)?(\(|\[)") assert_no_xrange_re = re.compile(r"\s*xrange\s*\(") @@ -109,23 +89,6 @@ class BaseASTChecker(ast.NodeVisitor): return False -def no_translate_debug_logs(logical_line, filename): - """Check for 'LOG.debug(u._(' - - 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. - - B313 - """ - if logical_line.startswith("LOG.debug(u._("): - yield(0, "B313 Don't translate debug level logs") - - class CheckLoggingFormatArgs(BaseASTChecker): """Check for improper use of logging format arguments. @@ -191,30 +154,6 @@ class CheckLoggingFormatArgs(BaseASTChecker): return super(CheckLoggingFormatArgs, self).generic_visit(node) -def validate_log_translations(logical_line, physical_line, filename): - # Translations are not required in the test directories. - if ("barbican/tests" in filename or "functional" in filename): - return - if pep8.noqa(physical_line): - return - msg = "B316: LOG.critical messages require translations `_LC()`!" - if log_translation_LC.match(logical_line): - yield (0, msg) - msg = ("B316: LOG.error and LOG.exception messages require translations " - "`_LE()`!") - if log_translation_LE.match(logical_line): - yield (0, msg) - msg = "B316: LOG.info messages require translations `_LI()`!" - if log_translation_LI.match(logical_line): - yield (0, msg) - msg = "B316: LOG.warning messages require translations `_LW()`!" - if log_translation_LW.match(logical_line): - yield (0, msg) - msg = "B316: Log messages require translations!" - if log_translation.match(logical_line): - yield (0, msg) - - class CheckForStrUnicodeExc(BaseASTChecker): """Checks for the use of str() or unicode() on an exception. @@ -262,28 +201,6 @@ class CheckForStrUnicodeExc(BaseASTChecker): super(CheckForStrUnicodeExc, self).generic_visit(node) -class CheckForTransAdd(BaseASTChecker): - """Checks for the use of concatenation on a translated string. - - Translations should not be concatenated with other strings, but - should instead include the string being added to the translated - string to give the translators the most information. - """ - - CHECK_DESC = ('B315 Translated messages cannot be concatenated. ' - 'String should be included in translated message.') - - TRANS_FUNC = ['_', '_LI', '_LW', '_LE', '_LC'] - - 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) - super(CheckForTransAdd, self).generic_visit(node) - - def check_oslo_namespace_imports(logical_line, physical_line, filename): """'oslo_' should be used instead of 'oslo.' @@ -365,11 +282,8 @@ def validate_assertIsNotNone(logical_line): def factory(register): - register(validate_log_translations) - register(no_translate_debug_logs) register(CheckForStrUnicodeExc) register(CheckLoggingFormatArgs) - register(CheckForTransAdd) register(check_oslo_namespace_imports) register(dict_constructor_with_list_copy) register(no_xrange) diff --git a/barbican/tests/test_hacking.py b/barbican/tests/test_hacking.py index 9a85893b7..228bdce26 100644 --- a/barbican/tests/test_hacking.py +++ b/barbican/tests/test_hacking.py @@ -12,7 +12,6 @@ # License for the specific language governing permissions and limitations # under the License. -import sys import textwrap import ddt @@ -57,16 +56,6 @@ class HackingTestCase(utils.BaseTestCase): should pass. """ - def test_no_translate_debug_logs(self): - self.assertEqual(1, len(list(checks.no_translate_debug_logs( - "LOG.debug(u._('foo'))", "barbican/scheduler/foo.py")))) - - self.assertEqual(0, len(list(checks.no_translate_debug_logs( - "LOG.debug('foo')", "barbican/scheduler/foo.py")))) - - self.assertEqual(0, len(list(checks.no_translate_debug_logs( - "LOG.info(_('foo'))", "barbican/scheduler/foo.py")))) - # We are patching pep8 so that only the check under test is actually # installed. @mock.patch('pep8._checks', @@ -187,50 +176,6 @@ class HackingTestCase(utils.BaseTestCase): errors = [(8, 20, 'B314'), (8, 33, 'B314'), (9, 16, 'B314')] self._assert_has_errors(code, checker, expected_errors=errors) - def test_trans_add(self): - - checker = checks.CheckForTransAdd - code = """ - def fake_tran(msg): - return msg - - - _ = 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 - """ - - # Python 3.4.0 introduced a change to the column calculation during AST - # parsing. This was reversed in Python 3.4.3, hence the version-based - # expected value calculation. See #1499743 for more background. - if sys.version_info < (3, 4, 0) or sys.version_info >= (3, 4, 3): - errors = [(13, 10, 'B315'), (14, 10, 'B315'), (15, 10, 'B315'), - (16, 10, 'B315'), (17, 10, 'B315'), (18, 24, 'B315')] - else: - errors = [(13, 11, 'B315'), (14, 13, 'B315'), (15, 13, 'B315'), - (16, 13, 'B315'), (17, 13, 'B315'), (18, 25, 'B315')] - self._assert_has_errors(code, checker, expected_errors=errors) - - code = """ - def f(a, b): - msg = 'test' + 'add me' - return msg - """ - errors = [] - self._assert_has_errors(code, checker, expected_errors=errors) - def test_dict_constructor_with_list_copy(self): self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy( " dict([(i, connect_info[i])"))))