Fix H904 hacking_delayed_string_interpolation

The regex for this check was wrong, resulting in misses. This fixes the
checks for string interpolation and adds unit tests to make sure we are
actually handling the cases we expect to be.

Change-Id: Id61094bb8ee8e93275c51c53caeb9ca27252b144
Signed-off-by: Sean McGinnis <sean.mcginnis@gmail.com>

hacking/checks/other.py

@@ -15,9 +15,8 @@ import re
 from hacking import core
 
 
-log_string_interpolation = re.compile(r".*LOG\.(?:error|warn|warning|info"
-                                      r"|critical|exception|debug)"
-                                      r"\([^,]*%[^,]*[,)]")
+log_string = re.compile(r".*LOG\.(?:error|warn|warning|info"
+                        r"|critical|exception|debug)")
 
 
 @core.flake8ext
@@ -50,5 +49,9 @@ def hacking_delayed_string_interpolation(logical_line, noqa):
     if noqa:
         return
 
-    if log_string_interpolation.match(logical_line):
-        yield 0, msg
+    if log_string.match(logical_line):
+        # Line is a log statement, strip out strings and see if % is used,
+        # just to make sure we don't match on a format specifier in a string.
+        line = re.sub(r"[\"'].+?[\"']", '', logical_line)
+        if '%' in line:
+            yield 0, msg

hacking/tests/checks/test_other.py

@@ -0,0 +1,39 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import ddt
+
+from hacking.checks import other
+from hacking import tests
+
+
+@ddt.ddt
+class OthersTestCase(tests.TestCase):
+    """This tests hacking checks from the 'other' group."""
+
+    @ddt.unpack
+    @ddt.data(
+        (1, 'LOG.debug("Test %s" % foo)', None),
+        (0, 'LOG.info("Test %s", foo)', None),
+        (0, 'LOG.error("Test %s" % foo)', '# noqa'),
+        (1, 'LOG.debug("Test %s" % "foo")', None),
+        (0, 'LOG.debug("Test %s", "foo")', None),
+        (0, "LOG.warning('Testing some stuff')", None))
+    def test_H904_hacking_delayed_string_interpolation(
+            self, err_count, line, noqa):
+        if err_count > 0:
+            self.assertCheckFails(other.hacking_delayed_string_interpolation,
+                                  line, noqa)
+        else:
+            self.assertCheckPasses(other.hacking_delayed_string_interpolation,
+                                   line, noqa)

test-requirements.txt

@@ -7,6 +7,7 @@ python-subunit>=1.0.0 # Apache-2.0/BSD
 stestr>=2.0.0 # Apache-2.0
 testscenarios>=0.4 # Apache-2.0/BSD
 testtools>=2.2.0 # MIT
+ddt>=1.2.1 # MIT
 
 # hacking doesn't use this anywhere, but nova imports this in nova/__init__.py
 # since eventlet is such a common universal import, add it to the hacking test