Hacking rule to check i18n usage

* Detect neutron.i18n import (neutron._i18n is recommended)
* Check builtins _ usage
* 'builtins = _' in tox.ini is no longer required.
* Introduce hacking rule doctest framework.
  Newly added check_builtins_gettext() hacking check takes
  token as argument. It is not a good idea to pass a tokenized
  line manually. Instead it is reasonable to use docstring based
  tests used in hacking repo.

Change-Id: Ib7464658fc4c8a6f1b03af6ab46f0bd3ee0bfb18

HACKING.rst

@@ -25,6 +25,8 @@ Neutron Specific Commandments
          assertEqual(observed_http_code, expected_http_code).
 - [N333] Validate that LOG.warning is used instead of LOG.warn. The latter
   is deprecated.
+- [N340] Check usage of <module>.i18n (and neutron.i18n)
+- [N341] Check usage of _ from python builtins
 
 Creating Unit Tests
 -------------------

neutron/__init__.py

@@ -25,6 +25,7 @@ else:
     gettext.install('neutron')
 
 
+# flake8: noqa
 six.moves.builtins.__dict__['_'] = removals.remove(
     message='Builtin _ translation function is deprecated in OpenStack; '
             'use the function from _i18n module for your project.')(_)

neutron/agent/l3/dvr_local_router.py

@@ -71,7 +71,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
         if self.rtr_fip_subnet is None:
             self.rtr_fip_subnet = self.fip_ns.local_subnets.allocate(
                 self.router_id)
-        rtr_2_fip, _ = self.rtr_fip_subnet.get_pair()
+        rtr_2_fip, __ = self.rtr_fip_subnet.get_pair()
         device = ip_lib.IPDevice(fip_2_rtr_name, namespace=fip_ns_name)
         device.route.add_route(fip_cidr, str(rtr_2_fip.ip))
         interface_name = (

neutron/db/bgp_db.py

@@ -25,6 +25,7 @@ from sqlalchemy.orm import exc as sa_exc
 
 from neutron_lib import constants as lib_consts
 
+from neutron._i18n import _
 from neutron.api.v2 import attributes as attr
 from neutron.common import exceptions as n_exc
 from neutron.db import address_scope_db

neutron/db/external_net_db.py

@@ -19,6 +19,7 @@ from sqlalchemy.orm import exc
 from sqlalchemy import sql
 from sqlalchemy.sql import expression as expr
 
+from neutron._i18n import _
 from neutron.api.v2 import attributes
 from neutron.callbacks import events
 from neutron.callbacks import exceptions as c_exc

neutron/db/sqlalchemytypes.py

@@ -15,6 +15,8 @@
 import netaddr
 from sqlalchemy import types
 
+from neutron._i18n import _
+
 
 class IPAddress(types.TypeDecorator):
 

neutron/hacking/checks.py

@@ -12,11 +12,23 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import os
 import re
 
 import pep8
 import six
 
+
+def flake8ext(f):
+    """Decorator to indicate flake8 extension.
+
+    This is borrowed from hacking.core.flake8ext(), but at now it is used
+    only for unit tests to know which are neutron flake8 extensions.
+    """
+    f.name = __name__
+    return f
+
+
 # Guidelines for writing new hacking checks
 #
 #  - Use only for Neutron specific tests. OpenStack general tests
@@ -59,6 +71,7 @@ log_warn = re.compile(
 contextlib_nested = re.compile(r"^with (contextlib\.)?nested\(")
 
 
+@flake8ext
 def validate_log_translations(logical_line, physical_line, filename):
     # Translations are not required in the test directory
     if "neutron/tests" in filename:
@@ -71,6 +84,7 @@ def validate_log_translations(logical_line, physical_line, filename):
         yield (0, msg)
 
 
+@flake8ext
 def use_jsonutils(logical_line, filename):
     msg = "N321: jsonutils.%(fun)s must be used instead of json.%(fun)s"
 
@@ -93,6 +107,7 @@ def use_jsonutils(logical_line, filename):
                 yield (pos, msg % {'fun': f[:-1]})
 
 
+@flake8ext
 def no_translate_debug_logs(logical_line, filename):
     """Check for 'LOG.debug(_(' and 'LOG.debug(_Lx('
 
@@ -108,6 +123,7 @@ def no_translate_debug_logs(logical_line, filename):
             yield(0, "N319 Don't translate debug level logs")
 
 
+@flake8ext
 def check_assert_called_once_with(logical_line, filename):
     # Try to detect unintended calls of nonexistent mock methods like:
     #    assert_called_once
@@ -131,6 +147,7 @@ def check_assert_called_once_with(logical_line, filename):
             yield (0, msg)
 
 
+@flake8ext
 def check_no_contextlib_nested(logical_line, filename):
     msg = ("N324: contextlib.nested is deprecated. With Python 2.7 and later "
            "the with-statement supports multiple nested objects. See https://"
@@ -141,12 +158,14 @@ def check_no_contextlib_nested(logical_line, filename):
         yield(0, msg)
 
 
+@flake8ext
 def check_python3_xrange(logical_line):
     if re.search(r"\bxrange\s*\(", logical_line):
         yield(0, "N325: Do not use xrange. Use range, or six.moves.range for "
                  "large loops.")
 
 
+@flake8ext
 def check_no_basestring(logical_line):
     if re.search(r"\bbasestring\b", logical_line):
         msg = ("N326: basestring is not Python3-compatible, use "
@@ -154,12 +173,14 @@ def check_no_basestring(logical_line):
         yield(0, msg)
 
 
+@flake8ext
 def check_python3_no_iteritems(logical_line):
     if re.search(r".*\.iteritems\(\)", logical_line):
         msg = ("N327: Use six.iteritems() instead of dict.iteritems().")
         yield(0, msg)
 
 
+@flake8ext
 def check_asserttrue(logical_line, filename):
     if 'neutron/tests/' in filename:
         if re.search(r"assertEqual\(\s*True,[^,]*(,[^,]*)?\)", logical_line):
@@ -172,12 +193,14 @@ def check_asserttrue(logical_line, filename):
             yield (0, msg)
 
 
+@flake8ext
 def no_mutable_default_args(logical_line):
     msg = "N329: Method's default argument shouldn't be mutable!"
     if mutable_default_args.match(logical_line):
         yield (0, msg)
 
 
+@flake8ext
 def check_assertfalse(logical_line, filename):
     if 'neutron/tests/' in filename:
         if re.search(r"assertEqual\(\s*False,[^,]*(,[^,]*)?\)", logical_line):
@@ -190,6 +213,7 @@ def check_assertfalse(logical_line, filename):
             yield (0, msg)
 
 
+@flake8ext
 def check_assertempty(logical_line, filename):
     if 'neutron/tests/' in filename:
         msg = ("N330: Use assertEqual(*empty*, observed) instead of "
@@ -201,6 +225,7 @@ def check_assertempty(logical_line, filename):
             yield (0, msg)
 
 
+@flake8ext
 def check_assertisinstance(logical_line, filename):
     if 'neutron/tests/' in filename:
         if re.search(r"assertTrue\(\s*isinstance\(\s*[^,]*,\s*[^,]*\)\)",
@@ -210,6 +235,7 @@ def check_assertisinstance(logical_line, filename):
             yield (0, msg)
 
 
+@flake8ext
 def check_assertequal_for_httpcode(logical_line, filename):
     msg = ("N332: Use assertEqual(expected_http_code, observed_http_code) "
            "instead of assertEqual(observed_http_code, expected_http_code)")
@@ -219,12 +245,82 @@ def check_assertequal_for_httpcode(logical_line, filename):
             yield (0, msg)
 
 
+@flake8ext
 def check_log_warn_deprecated(logical_line, filename):
     msg = "N333: Use LOG.warning due to compatibility with py3"
     if log_warn.match(logical_line):
         yield (0, msg)
 
 
+@flake8ext
+def check_oslo_i18n_wrapper(logical_line, filename, noqa):
+    """Check for neutron.i18n usage.
+
+    Okay(neutron/foo/bar.py): from neutron._i18n import _
+    Okay(neutron_lbaas/foo/bar.py): from neutron_lbaas._i18n import _
+    N340(neutron/foo/bar.py): from neutron.i18n import _
+    N340(neutron_lbaas/foo/bar.py): from neutron_lbaas.i18n import _
+    N340(neutron_lbaas/foo/bar.py): from neutron.i18n import _
+    N340(neutron_lbaas/foo/bar.py): from neutron._i18n import _
+    Okay(neutron/foo/bar.py): from neutron.i18n import _  # noqa
+    """
+
+    if noqa:
+        return
+
+    split_line = logical_line.split()
+    modulename = os.path.normpath(filename).split('/')[0]
+    bad_i18n_module = '%s.i18n' % modulename
+
+    if (len(split_line) > 1 and split_line[0] in ('import', 'from')):
+        if (split_line[1] == bad_i18n_module or
+            modulename != 'neutron' and split_line[1] in ('neutron.i18n',
+                                                          'neutron._i18n')):
+            msg = ("N340: %(found)s is found. Use %(module)s._i18n instead."
+                   % {'found': split_line[1], 'module': modulename})
+            yield (0, msg)
+
+
+@flake8ext
+def check_builtins_gettext(logical_line, tokens, filename, lines, noqa):
+    """Check usage of builtins gettext _().
+
+    Okay(neutron/foo.py): from neutron._i18n import _\n_('foo')
+    N341(neutron/foo.py): _('foo')
+    Okay(neutron/_i18n.py): _('foo')
+    Okay(neutron/i18n.py): _('foo')
+    Okay(neutron/foo.py): _('foo')  # noqa
+    """
+
+    if noqa:
+        return
+
+    modulename = os.path.normpath(filename).split('/')[0]
+
+    if '%s/tests' % modulename in filename:
+        return
+
+    if os.path.basename(filename) in ('i18n.py', '_i18n.py'):
+        return
+
+    token_values = [t[1] for t in tokens]
+    i18n_wrapper = '%s._i18n' % modulename
+
+    if '_' in token_values:
+        i18n_import_line_found = False
+        for line in lines:
+            split_line = [elm.rstrip(',') for elm in line.split()]
+            if (len(split_line) > 1 and split_line[0] == 'from' and
+                    split_line[1] == i18n_wrapper and
+                    '_' in split_line):
+                i18n_import_line_found = True
+                break
+        if not i18n_import_line_found:
+            msg = ("N341: _ from python builtins module is used. "
+                   "Use _ from %s instead." % i18n_wrapper)
+            yield (0, msg)
+
+
 def factory(register):
     register(validate_log_translations)
     register(use_jsonutils)
@@ -241,3 +337,5 @@ def factory(register):
     register(check_assertisinstance)
     register(check_assertequal_for_httpcode)
     register(check_log_warn_deprecated)
+    register(check_oslo_i18n_wrapper)
+    register(check_builtins_gettext)

neutron/pecan_wsgi/controllers/extensions.py

@@ -14,6 +14,7 @@
 
 import pecan
 
+from neutron._i18n import _
 from neutron.api import extensions
 from neutron.pecan_wsgi.controllers import utils
 

neutron/pecan_wsgi/controllers/resource.py

@@ -16,8 +16,8 @@ from oslo_log import log as logging
 import pecan
 from pecan import request
 
+from neutron._i18n import _LW
 from neutron.api import api_common
-from neutron.i18n import _LW
 from neutron import manager
 from neutron.pecan_wsgi.controllers import utils
 

neutron/plugins/ml2/drivers/agent/config.py

@@ -16,6 +16,7 @@
 
 from oslo_config import cfg
 
+from neutron._i18n import _
 from neutron.agent.common import config
 
 agent_opts = [

neutron/plugins/ml2/drivers/l2pop/mech_driver.py

@@ -185,7 +185,7 @@ class L2populationMechanismDriver(api.MechanismDriver):
 
     def _get_tunnels(self, tunnel_network_ports, exclude_host):
         agents = {}
-        for _, agent in tunnel_network_ports:
+        for __, agent in tunnel_network_ports:
             if agent.host == exclude_host:
                 continue
 

neutron/plugins/ml2/drivers/macvtap/agent/config.py

@@ -16,6 +16,8 @@
 
 from oslo_config import cfg
 
+from neutron._i18n import _
+
 DEFAULT_INTERFACE_MAPPINGS = []
 
 macvtap_opts = [

neutron/tests/unit/hacking/test_checks.py

@@ -10,7 +10,15 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import re
+
+from flake8 import engine
+from hacking.tests import test_doctest as hacking_doctest
+import pep8
+import testscenarios
 import testtools
+from testtools import content
+from testtools import matchers
 
 from neutron.hacking import checks
 from neutron.tests import base
@@ -273,3 +281,84 @@ class HackingTestCase(base.BaseTestCase):
         self.assertEqual(
             0, len(list(checks.check_assertequal_for_httpcode(pass_code,
                                         "neutron/tests/test_assert.py"))))
+
+# The following is borrowed from hacking/tests/test_doctest.py.
+# Tests defined in docstring is easier to understand
+# in some cases, for example, hacking rules which take tokens as argument.
+
+# TODO(amotoki): Migrate existing unit tests above to docstring tests.
+# NOTE(amotoki): Is it better to enhance HackingDocTestCase in hacking repo to
+# pass filename to pep8.Checker so that we can reuse it in this test.
+# I am not sure whether unit test class is public.
+
+SELFTEST_REGEX = re.compile(r'\b(Okay|N\d{3})(\((\S+)\))?:\s(.*)')
+
+
+# Each scenario is (name, dict(filename=..., lines=.., options=..., code=...))
+file_cases = []
+
+
+class HackingDocTestCase(hacking_doctest.HackingTestCase):
+
+    scenarios = file_cases
+
+    def test_pep8(self):
+
+        # NOTE(jecarey): Add tests marked as off_by_default to enable testing
+        turn_on = set(['H106'])
+        if self.options.select:
+            turn_on.update(self.options.select)
+        self.options.select = tuple(turn_on)
+
+        report = pep8.BaseReport(self.options)
+        checker = pep8.Checker(filename=self.filename, lines=self.lines,
+                               options=self.options, report=report)
+        checker.check_all()
+        self.addDetail('doctest', content.text_content(self.raw))
+        if self.code == 'Okay':
+            self.assertThat(
+                len(report.counters),
+                matchers.Not(matchers.GreaterThan(
+                    len(self.options.benchmark_keys))),
+                "incorrectly found %s" % ', '.join(
+                    [key for key in report.counters
+                     if key not in self.options.benchmark_keys]))
+        else:
+            self.addDetail('reason',
+                           content.text_content("Failed to trigger rule %s" %
+                                                self.code))
+            self.assertIn(self.code, report.counters)
+
+
+def _get_lines(check):
+    for line in check.__doc__.splitlines():
+        line = line.lstrip()
+        match = SELFTEST_REGEX.match(line)
+        if match is None:
+            continue
+        yield (line, match.groups())
+
+
+def load_tests(loader, tests, pattern):
+
+    flake8_style = engine.get_style_guide(parse_argv=False,
+                                          # Ignore H104 otherwise it's
+                                          # raised on doctests.
+                                          ignore=('F', 'H104'))
+    options = flake8_style.options
+
+    for name, check in checks.__dict__.items():
+        if not hasattr(check, 'name'):
+            continue
+        if check.name != checks.__name__:
+            continue
+        if not check.__doc__:
+            continue
+        for (lineno, (raw, line)) in enumerate(_get_lines(check)):
+            code, __, filename, source = line
+            lines = [part.replace(r'\t', '\t') + '\n'
+                     for part in source.split(r'\n')]
+            file_cases.append(("%s-line-%s" % (name, lineno),
+                              dict(lines=lines, raw=raw, options=options,
+                                   code=code, filename=filename)))
+    return testscenarios.load_tests_apply_scenarios(loader, tests, pattern)

tox.ini

@@ -133,14 +133,13 @@ commands = sphinx-build -W -b html doc/source doc/build/html
 # H405 multi line docstring summary not separated with an empty line
 ignore = E125,E126,E128,E129,E265,H404,H405
 show-source = true
-builtins = _
 # neutron/tests/tempest needs to be excluded so long as it continues
 # to be copied directly from tempest, since tempest and neutron do not
 # share a flake8 configuration.
 exclude = ./.*,build,dist,neutron/openstack/common/*,neutron/tests/tempest
 
 [hacking]
-import_exceptions = neutron.i18n, neutron._i18n
+import_exceptions = neutron._i18n
 local-check-factory = neutron.hacking.checks.factory
 
 [testenv:genconfig]