diff --git a/HACKING.rst b/HACKING.rst index 374bc06c..caec6945 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -15,13 +15,10 @@ Masakari Specific Commandments calls to datetime.datetime.utcnow() to make it easy to override its return value in tests - [M303] capitalize help string Config parameter help strings should have a capitalized first letter -- [M304] vim configuration should not be kept in source files. - [M305] Change assertTrue(isinstance(A, B)) by optimal assert like assertIsInstance(A, B). - [M306] Change assertEqual(type(A), B) by optimal assert like assertIsInstance(A, B) -- [M307] Change assertEqual(A, None) or assertEqual(None, A) by optimal assert like - assertIsNone(A) - [M308] Validate that debug level logs are not translated. - [M309] Don't import translation in tests - [M310] Setting CONF.* attributes directly in tests is forbidden. Use @@ -45,5 +42,4 @@ Masakari Specific Commandments - [M327] Python 3: do not use dict.iterkeys. - [M328] Python 3: do not use dict.itervalues. - [M329] Deprecated library function os.popen() -- [M330] String interpolation should be delayed at logging calls. - [M331] LOG.warn is deprecated. Enforce use of LOG.warning. diff --git a/masakari/hacking/checks.py b/masakari/hacking/checks.py index 72c54496..baaac042 100644 --- a/masakari/hacking/checks.py +++ b/masakari/hacking/checks.py @@ -35,7 +35,6 @@ UNDERSCORE_IMPORT_FILES = [] session_check = re.compile(r"\w*def [a-zA-Z0-9].*[(].*session.*[)]") cfg_re = re.compile(r".*\scfg\.") cfg_opt_re = re.compile(r".*[\s\[]cfg\.[a-zA-Z]*Opt\(") -vi_header_re = re.compile(r"^#\s+vim?:.+") asse_trueinst_re = re.compile( r"(.)*assertTrue\(isinstance\((\w|\.|\'|\"|\[|\])+, " "(\w|\.|\'|\"|\[|\])+\)\)") @@ -86,9 +85,6 @@ spawn_re = re.compile( contextlib_nested = re.compile(r"^with (contextlib\.)?nested\(") doubled_words_re = re.compile( r"\b(then?|[iao]n|i[fst]|but|f?or|at|and|[dt]o)\s+\1\b") -log_string_interpolation = re.compile(r".*LOG\.(error|warning|info" - r"|critical|exception|debug)" - r"\([^,]*%[^,]*[,)]") def no_db_session_in_public_api(logical_line, filename): @@ -124,20 +120,6 @@ def capital_cfg_help(logical_line, tokens): yield(0, msg) -def no_vi_headers(physical_line, line_number, lines): - """Check for vi editor configuration in source files. - - By default vi modelines can only appear in the first or - last 5 lines of a source file. - - M304 - """ - # NOTE(abhishekk): line_number is 1-indexed - if line_number <= 5 or line_number > len(lines) - 5: - if vi_header_re.match(physical_line): - return 0, "M304: Don't put vi configuration in source files" - - def assert_true_instance(logical_line): """Check for assertTrue(isinstance(a, b)) sentences @@ -157,18 +139,6 @@ def assert_equal_type(logical_line): yield (0, "M306: assertEqual(type(A), B) sentences not allowed") -def assert_equal_none(logical_line): - """Check for assertEqual(A, None) or assertEqual(None, A) sentences - - M307 - """ - res = (asse_equal_start_with_none_re.search(logical_line) or - asse_equal_end_with_none_re.search(logical_line)) - if res: - yield (0, "M307: assertEqual(A, None) or assertEqual(None, A) " - "sentences not allowed") - - def no_translate_debug_logs(logical_line, filename): """Check for 'LOG.debug(_(' @@ -392,27 +362,6 @@ def no_os_popen(logical_line): 'Replace it using subprocess module. ') -def check_delayed_string_interpolation(logical_line, filename, noqa): - """M330 String interpolation should be delayed at logging calls. - - M330: LOG.debug('Example: %s' % 'bad') - Okay: LOG.debug('Example: %s', 'good') - """ - msg = ("M330 String interpolation should be delayed to be " - "handled by the logging code, rather than being done " - "at the point of the logging call. " - "Use ',' instead of '%'.") - - if noqa: - return - - if '/tests/' in filename: - return - - if log_string_interpolation.match(logical_line): - yield(logical_line.index('%'), msg) - - def no_log_warn(logical_line): """Disallow 'LOG.warn(' @@ -431,11 +380,9 @@ def factory(register): register(no_db_session_in_public_api) register(use_timeutils_utcnow) register(capital_cfg_help) - register(no_vi_headers) register(no_import_translation_in_tests) register(assert_true_instance) register(assert_equal_type) - register(assert_equal_none) register(assert_raises_regexp) register(no_translate_debug_logs) register(no_setting_conf_directly_in_tests) @@ -453,5 +400,4 @@ def factory(register): register(check_python3_no_iterkeys) register(check_python3_no_itervalues) register(no_os_popen) - register(check_delayed_string_interpolation) register(no_log_warn) diff --git a/masakari/tests/unit/test_hacking.py b/masakari/tests/unit/test_hacking.py index aaff7249..de52b973 100644 --- a/masakari/tests/unit/test_hacking.py +++ b/masakari/tests/unit/test_hacking.py @@ -53,30 +53,6 @@ class HackingTestCase(test.NoDBTestCase): just assertTrue if the check is expected to fail and assertFalse if it should pass. """ - def test_no_vi_headers(self): - - lines = ['Line 1\n', 'Line 2\n', 'Line 3\n', 'Line 4\n', 'Line 5\n', - 'Line 6\n', 'Line 7\n', 'Line 8\n', 'Line 9\n', 'Line 10\n', - 'Line 11\n', 'Line 12\n', 'Line 13\n', 'Line14\n', 'Line15\n'] - - self.assertIsNone(checks.no_vi_headers( - "Test string foo", 1, lines)) - self.assertEqual(len(list(checks.no_vi_headers( - "# vim: et tabstop=4 shiftwidth=4 softtabstop=4", - 2, lines))), 2) - self.assertIsNone(checks.no_vi_headers( - "# vim: et tabstop=4 shiftwidth=4 softtabstop=4", - 6, lines)) - self.assertIsNone(checks.no_vi_headers( - "# vim: et tabstop=4 shiftwidth=4 softtabstop=4", - 9, lines)) - self.assertEqual(len(list(checks.no_vi_headers( - "# vim: et tabstop=4 shiftwidth=4 softtabstop=4", - 14, lines))), 2) - self.assertIsNone(checks.no_vi_headers( - "Test end string for vi", - 15, lines)) - def test_assert_true_instance(self): self.assertEqual(len(list(checks.assert_true_instance( "self.assertTrue(isinstance(e, " @@ -129,19 +105,7 @@ class HackingTestCase(test.NoDBTestCase): self.assertEqual(len(list(checks.assert_equal_in( "self.assertEqual(False, any(a==1 for a in b))"))), 0) - def test_assert_equal_none(self): - self.assertEqual(len(list(checks.assert_equal_none( - "self.assertEqual(A, None)"))), 1) - - self.assertEqual(len(list(checks.assert_equal_none( - "self.assertEqual(None, A)"))), 1) - - self.assertEqual( - len(list(checks.assert_equal_none("self.assertIsNone()"))), 0) - def test_assert_true_or_false_with_in_or_not_in(self): - self.assertEqual(len(list(checks.assert_equal_none( - "self.assertEqual(A, None)"))), 1) self.assertEqual(len(list(checks.assert_true_or_false_with_in( "self.assertTrue(A in B)"))), 1) @@ -456,58 +420,6 @@ class HackingTestCase(test.NoDBTestCase): self._assert_has_errors(code, checks.no_os_popen, expected_errors=errors) - def test_check_delayed_string_interpolation(self): - checker = checks.check_delayed_string_interpolation - code = """ - msg_w = ('Test string (%s)') - msg_i = 'Test string (%s)' - value = 'test' - - LOG.error(("Test string (%s)") % value) - LOG.warning(msg_w % 'test%string') - LOG.info(msg_i % - "test%string%info") - LOG.critical( - ('Test string (%s)') % value, - instance=instance) - LOG.exception((" 'Test quotation %s' \"Test\"") % 'test') - LOG.debug(' "Test quotation %s" \'Test\'' % "test") - LOG.debug('Tesing %(test)s' % - {'test': ','.join( - ['%s=%s' % (name, value) - for name, value in test.items()])}) - """ - - expected_errors = [(5, 31, 'M330'), (6, 18, 'M330'), (7, 15, 'M330'), - (10, 25, 'M330'), (12, 46, 'M330'), - (13, 40, 'M330'), (14, 28, 'M330')] - self._assert_has_errors(code, checker, expected_errors=expected_errors) - self._assert_has_no_errors( - code, checker, filename='masakari/tests/unit/test_hacking.py') - - code = """ - msg_w = ('Test string (%s)') - msg_i = 'Test string (%s)' - value = 'test' - - LOG.error(("Test string (%s)"), value) - LOG.error(("Test string (%s)") % value) # noqa - LOG.warn(('Test string (%s)'), - value) - LOG.info(msg_i, - "test%string%info") - LOG.critical( - ('Test string (%s)'), value, - instance=instance) - LOG.exception((" 'Test quotation %s' \"Test\""), 'test') - LOG.debug(' "Test quotation %s" \'Test\'', "test") - LOG.debug('Tesing %(test)s', - {'test': ','.join( - ['%s=%s' % (name, value) - for name, value in test.items()])}) - """ - self._assert_has_no_errors(code, checker) - def test_no_log_warn(self): code = """ LOG.warn("LOG.warn is deprecated") diff --git a/tox.ini b/tox.ini index ae7083de..8cde388f 100644 --- a/tox.ini +++ b/tox.ini @@ -61,6 +61,12 @@ commands = oslo_debug_helper {posargs} # E123, E125 skipped as they are invalid PEP-8. show-source = True + +# The below hacking rules by default are disabled should be enabled: +# [H106] Don't put vim configuration in source files. +# [H203] Use assertIs(Not)None to check for None. +# [H904] Delay string interpolations at logging calls. +enable-extensions = H106,H203,H904 ignore = E123,E125,E128,H405 builtins = _ exclude=.venv,.git,.tox,dist,doc,*lib/python*,*egg,build