Enable global hacking checks and remove local checks
In newer hacking version 0.12.0 [1], we can enable some of the
non-default hacking rules (one by one), which are disabled by
default. The enabled rules are the following:
* [H106] Don’t put vim configuration in source files (off by default).
* [H203] Use assertIs(Not)None to check for None (off by default).
* [H904] Delay string interpolations at logging calls (off by default).
Enabled these hacking rules by adding them in the list of
'enable-extensions' in tox.ini [flake8] section. Removed the local
implementation of those hacking rules from hacking/checks.py.
The test-requirements.txt is already updated to use the newer
hacking version 0.12.0 with this commit:
cc44a33f3d
[1] See "Enabling off-by-default checks" section:
https://pypi.python.org/pypi/hacking/0.12.0
Change-Id: Ieccd5a84ebd80ba3313016c9caeb036eaa37769b
This commit is contained in:
parent
b26ba43f72
commit
66c607c6e8
|
@ -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.
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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")
|
||||
|
|
6
tox.ini
6
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
|
||||
|
|
Loading…
Reference in New Issue