From ebc0219a50979ba11ef7a8015270fc215191f7af Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 31 Aug 2016 10:52:41 +0100 Subject: [PATCH] hacking: Always use 'assertIs(Not)None' This is per the OpenStack style guidelines. Change-Id: Ia706045fe3524b6b5e1db0140672776d481a0c01 --- HACKING.rst | 4 ++-- nova/hacking/checks.py | 21 +++++++++++++-------- nova/tests/unit/test_hacking.py | 14 ++++++++++++-- 3 files changed, 27 insertions(+), 12 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 4ef3f28752d4..ad0eb0338bea 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -26,8 +26,8 @@ Nova Specific Commandments assertIsInstance(A, B). - [N317] Change assertEqual(type(A), B) by optimal assert like assertIsInstance(A, B) -- [N318] Change assertEqual(A, None) or assertEqual(None, A) by optimal assert like - assertIsNone(A) +- [N318] Change assertEqual(A, None) or assertIs(A, None) to optimal assert + like assertIsNone(A) - [N319] Validate that debug level logs are not translated. - [N320] Setting CONF.* attributes directly in tests is forbidden. Use self.flags(option=value) instead. diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 8ea17908403e..21cf62424943 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -58,10 +58,6 @@ asse_equal_in_end_with_true_or_false_re = re.compile(r"assertEqual\(" r"(\w|[][.'\"])+ in (\w|[][.'\", ])+, (True|False)\)") asse_equal_in_start_with_true_or_false_re = re.compile(r"assertEqual\(" r"(True|False), (\w|[][.'\"])+ in (\w|[][.'\", ])+\)") -asse_equal_end_with_none_re = re.compile( - r"assertEqual\(.*?,\s+None\)$") -asse_equal_start_with_none_re = re.compile( - r"assertEqual\(None,") # NOTE(snikitin): Next two regexes weren't united to one for more readability. # asse_true_false_with_in_or_not_in regex checks # assertTrue/False(A in B) cases where B argument has no spaces @@ -283,11 +279,20 @@ def assert_equal_none(logical_line): N318 """ - res = (asse_equal_start_with_none_re.search(logical_line) or - asse_equal_end_with_none_re.search(logical_line)) - if res: + _start_re = re.compile(r"assertEqual\(.*?,\s+None\)$") + _end_re = re.compile(r"assertEqual\(None,") + + if _start_re.search(logical_line) or _end_re.search(logical_line): yield (0, "N318: assertEqual(A, None) or assertEqual(None, A) " - "sentences not allowed") + "sentences not allowed. Use assertIsNone(A) instead.") + + _start_re = re.compile(r"assertIs(Not)?\(None,") + _end_re = re.compile(r"assertIs(Not)?\(.*,\s+None\)$") + + if _start_re.search(logical_line) or _end_re.search(logical_line): + yield (0, "N318: assertIsNot(A, None) or assertIsNot(None, A) must " + "not be used. Use assertIsNone(A) or assertIsNotNone(A) " + "instead.") def check_python3_xrange(logical_line): diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index c8007a9341cb..b490aa78bc63 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -168,9 +168,19 @@ class HackingTestCase(test.NoDBTestCase): 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.assertIs(A, None)"))), 1) + + self.assertEqual(len(list(checks.assert_equal_none( + "self.assertIsNot(A, None)"))), 1) + + self.assertEqual(len(list(checks.assert_equal_none( + "self.assertIs(None, A)"))), 1) + + self.assertEqual(len(list(checks.assert_equal_none( + "self.assertIsNot(None, A)"))), 1) + + def test_assert_true_or_false_with_in_or_not_in(self): self.assertEqual(len(list(checks.assert_true_or_false_with_in( "self.assertTrue(A in B)"))), 1)