Enhance safe template linter.

- Lint directories and files in sorted order.
- Lint python blocks in Mako.

scripts/safe_template_linter.py

@@ -752,41 +752,6 @@ class BaseLinter(object):
 
     LINE_COMMENT_DELIM = None
 
-    def __init__(self):
-        """
-        Init method.
-        """
-        self._skip_dirs = (
-            '.git',
-            '.pycharm_helpers',
-            'common/static/xmodule/modules',
-            'perf_tests',
-            'node_modules',
-            'reports/diff_quality',
-            'scripts/tests/templates',
-            'spec',
-            'test_root',
-            'vendor',
-        )
-
-    def _is_skip_dir(self, skip_dirs, directory):
-        """
-        Determines whether a directory should be skipped or linted.
-
-        Arguments:
-            skip_dirs: The configured directories to be skipped.
-            directory: The current directory to be tested.
-
-        Returns:
-             True if the directory should be skipped, and False otherwise.
-
-        """
-        for skip_dir in skip_dirs:
-            skip_dir_regex = re.compile("(.*/)*{}(/.*)*".format(skip_dir))
-            if skip_dir_regex.match(directory) is not None:
-                return True
-        return False
-
     def _is_valid_directory(self, skip_dirs, directory):
         """
         Determines if the provided directory is a directory that could contain
@@ -800,7 +765,7 @@ class BaseLinter(object):
             True if this directory should be linted for violations and False
             otherwise.
         """
-        if self._is_skip_dir(skip_dirs, directory):
+        if is_skip_dir(skip_dirs, directory):
             return False
 
         return True
@@ -966,7 +931,7 @@ class UnderscoreTemplateLinter(BaseLinter):
         Init method.
         """
         super(UnderscoreTemplateLinter, self).__init__()
-        self._skip_underscore_dirs = self._skip_dirs + ('test',)
+        self._skip_underscore_dirs = SKIP_DIRS + ('test',)
 
     def process_file(self, directory, file_name):
         """
@@ -1081,8 +1046,8 @@ class JavaScriptLinter(BaseLinter):
         Init method.
         """
         super(JavaScriptLinter, self).__init__()
-        self._skip_javascript_dirs = self._skip_dirs + ('i18n', 'static/coffee')
-        self._skip_coffeescript_dirs = self._skip_dirs
+        self._skip_javascript_dirs = SKIP_DIRS + ('i18n', 'static/coffee')
+        self._skip_coffeescript_dirs = SKIP_DIRS
         self.underscore_linter = UnderscoreTemplateLinter()
 
     def process_file(self, directory, file_name):
@@ -1770,7 +1735,7 @@ class PythonLinter(BaseLinter):
         Init method.
         """
         super(PythonLinter, self).__init__()
-        self._skip_python_dirs = self._skip_dirs + ('tests', 'test/acceptance')
+        self._skip_python_dirs = SKIP_DIRS + ('tests', 'test/acceptance')
 
     def process_file(self, directory, file_name):
         """
@@ -1977,7 +1942,7 @@ class MakoTemplateLinter(BaseLinter):
             True if this directory should be linted for Mako template violations
             and False otherwise.
         """
-        if self._is_skip_dir(self._skip_dirs, directory):
+        if is_skip_dir(SKIP_DIRS, directory):
             return False
 
         # TODO: This is an imperfect guess concerning the Mako template
@@ -2001,6 +1966,7 @@ class MakoTemplateLinter(BaseLinter):
             return
         has_page_default = self._has_page_default(mako_template, results)
         self._check_mako_expressions(mako_template, has_page_default, results)
+        self._check_mako_python_blocks(mako_template, has_page_default, results)
         results.prepare_results(mako_template, line_comment_delim=self.LINE_COMMENT_DELIM)
 
     def _is_django_template(self, mako_template):
@@ -2139,6 +2105,30 @@ class MakoTemplateLinter(BaseLinter):
         self.javascript_linter.check_javascript_file_is_safe(javascript_code, javascript_results)
         self._shift_and_add_violations(javascript_results, start_offset, results)
 
+    def _check_mako_python_blocks(self, mako_template, has_page_default, results):
+        """
+        Searches for Mako python blocks and checks if they contain
+        violations.
+
+        Arguments:
+            mako_template: The contents of the Mako template.
+            has_page_default: True if the page is marked as default, False
+                otherwise.
+            results: A list of results into which violations will be added.
+
+        """
+        # Finds Python blocks such as <% ... %>, skipping other Mako start tags
+        # such as <%def> and <%page>.
+        python_block_regex = re.compile(r'<%\s(?P<code>.*?)%>', re.DOTALL)
+
+        for python_block_match in python_block_regex.finditer(mako_template):
+            self._check_expression_python(
+                python_code=python_block_match.group('code'),
+                start_offset=(python_block_match.start() + len('<% ')),
+                has_page_default=has_page_default,
+                results=results
+            )
+
     def _check_expression_python(self, python_code, start_offset, has_page_default, results):
         """
         Lint the Python inside a single Python expression in a Mako template.
@@ -2474,6 +2464,40 @@ class MakoTemplateLinter(BaseLinter):
         return expressions
 
 
+SKIP_DIRS = (
+    '.git',
+    '.pycharm_helpers',
+    'common/static/xmodule/modules',
+    'perf_tests',
+    'node_modules',
+    'reports/diff_quality',
+    'scripts/tests/templates',
+    'spec',
+    'test_root',
+    'vendor',
+)
+
+
+def is_skip_dir(skip_dirs, directory):
+    """
+    Determines whether a directory should be skipped or linted.
+
+    Arguments:
+        skip_dirs: The configured directories to be skipped.
+        directory: The current directory to be tested.
+
+    Returns:
+         True if the directory should be skipped, and False otherwise.
+
+    """
+    for skip_dir in skip_dirs:
+        skip_dir_regex = re.compile(
+            "(.*/)*{}(/.*)*".format(re.escape(skip_dir)))
+        if skip_dir_regex.match(directory) is not None:
+            return True
+    return False
+
+
 def _process_file(full_path, template_linters, options, summary_results, out):
     """
     For each linter, lints the provided file.  This means finding and printing
@@ -2495,28 +2519,25 @@ def _process_file(full_path, template_linters, options, summary_results, out):
         results.print_results(options, summary_results, out)
 
 
-def _process_current_walk(current_walk, template_linters, options, summary_results, out):
+def _process_os_dir(directory, files, template_linters, options, summary_results, out):
     """
-    For each linter, lints all the files in the current os walk.  This means
-    finding and printing violations.
+    Calls out to lint each file in the passed list of files.
 
     Arguments:
-        current_walk: A walk returned by os.walk().
+        directory: Directory being linted.
+        files: All files in the directory to be linted.
         template_linters: A list of linting objects.
         options: A list of the options.
         summary_results: A SummaryResults with a summary of the violations.
         out: output file
 
     """
-    num_violations = 0
-    walk_directory = os.path.normpath(current_walk[0])
-    walk_files = current_walk[2]
-    for walk_file in walk_files:
-        full_path = os.path.join(walk_directory, walk_file)
+    for current_file in sorted(files, key=lambda s: s.lower()):
+        full_path = os.path.join(directory, current_file)
         _process_file(full_path, template_linters, options, summary_results, out)
 
 
-def _process_os_walk(starting_dir, template_linters, options, summary_results, out):
+def _process_os_dirs(starting_dir, template_linters, options, summary_results, out):
     """
     For each linter, lints all the directories in the starting directory.
 
@@ -2528,9 +2549,12 @@ def _process_os_walk(starting_dir, template_linters, options, summary_results, o
         out: output file
 
     """
-    num_violations = 0
-    for current_walk in os.walk(starting_dir):
-        _process_current_walk(current_walk, template_linters, options, summary_results, out)
+    for root, dirs, files in os.walk(starting_dir):
+        if is_skip_dir(SKIP_DIRS, root):
+            del dirs
+            continue
+        dirs.sort(key=lambda s: s.lower())
+        _process_os_dir(root, files, template_linters, options, summary_results, out)
 
 
 def _lint(file_or_dir, template_linters, options, summary_results, out):
@@ -2555,7 +2579,7 @@ def _lint(file_or_dir, template_linters, options, summary_results, out):
                 directory = file_or_dir
             else:
                 raise ValueError("Path [{}] is not a valid file or directory.".format(file_or_dir))
-        _process_os_walk(directory, template_linters, options, summary_results, out)
+        _process_os_dirs(directory, template_linters, options, summary_results, out)
 
     summary_results.print_results(options, out)
 

scripts/safelint_thresholds.json

 {
     "rules": {
-        "javascript-concat-html": 230,
+        "javascript-concat-html": 229,
         "javascript-escape": 7,
-        "javascript-interpolate": 66,
-        "javascript-jquery-append": 114,
-        "javascript-jquery-html": 304,
+        "javascript-interpolate": 65,
+        "javascript-jquery-append": 113,
+        "javascript-jquery-html": 303,
         "javascript-jquery-insert-into-target": 26,
-        "javascript-jquery-insertion": 30,
+        "javascript-jquery-insertion": 28,
         "javascript-jquery-prepend": 12,
         "mako-html-entities": 0,
         "mako-invalid-html-filter": 33,
         "mako-invalid-js-filter": 222,
         "mako-js-html-string": 0,
         "mako-js-missing-quotes": 0,
-        "mako-missing-default": 234,
+        "mako-missing-default": 231,
         "mako-multiple-page-tags": 0,
         "mako-unknown-context": 0,
         "mako-unparseable-expression": 0,
         "mako-unwanted-html-filter": 0,
         "python-close-before-format": 0,
-        "python-concat-html": 28,
+        "python-concat-html": 27,
         "python-custom-escape": 13,
-        "python-deprecated-display-name": 53,
+        "python-deprecated-display-name": 57,
         "python-interpolate-html": 68,
         "python-parse-error": 0,
         "python-requires-html-or-text": 0,
-        "python-wrap-html": 279,
+        "python-wrap-html": 297,
         "underscore-not-escaped": 709
     },
-    "total": 2405
+    "total": 2426
 }

scripts/tests/test_safe_template_linter.py

@@ -9,7 +9,7 @@ from StringIO import StringIO
 import textwrap
 from unittest import TestCase
 
-from ..safe_template_linter import (
+from scripts.safe_template_linter import (
     _lint, FileResults, JavaScriptLinter, MakoTemplateLinter, ParseString,
     StringLines, PythonLinter, SummaryResults, UnderscoreTemplateLinter, Rules
 )
@@ -72,8 +72,14 @@ class TestLinter(TestCase):
             rules = data['rule']
         elif data['rule'] is not None:
             rules.append(data['rule'])
-        self.assertEqual(len(results.violations), len(rules))
         results.violations.sort(key=lambda violation: violation.sort_key())
+
+        # Print violations if the lengths are different.
+        if len(results.violations) != len(rules):
+            for violation in results.violations:
+                print("Found violation: {}".format(violation.rule))
+
+        self.assertEqual(len(results.violations), len(rules))
         for violation, rule in zip(results.violations, rules):
             self.assertEqual(violation.rule, rule)
 
@@ -92,6 +98,10 @@ class TestSafeTemplateLinter(TestCase):
         self.patch_is_valid_directory(UnderscoreTemplateLinter)
         self.patch_is_valid_directory(PythonLinter)
 
+        patcher = mock.patch('scripts.safe_template_linter.is_skip_dir', return_value=False)
+        patcher.start()
+        self.addCleanup(patcher.stop)
+
     def patch_is_valid_directory(self, linter_class):
         """
         Creates a mock patch for _is_valid_directory on a Linter to always
@@ -359,6 +369,22 @@ class TestMakoTemplateLinter(TestLinter):
 
     @data(
         {
+            # Python blocks between <% ... %> use the same Python linting as
+            # Mako expressions between ${ ... }. This single test verifies
+            # that these blocks are linted. The individual linting rules are
+            # tested in the Mako expression tests that follow.
+            'expression':
+                textwrap.dedent("""
+                    <%
+                        a_link_start = '<a class="link-courseURL" rel="external" href="'
+                        a_link_end = '">' + _("your course summary page") + '</a>'
+                        a_link = a_link_start + lms_link_for_about_page + a_link_end
+                        text = _("Introductions, prerequisites, FAQs that are used on %s (formatted in HTML)") % a_link
+                    %>
+                """),
+            'rule': [Rules.python_wrap_html, Rules.python_concat_html, Rules.python_wrap_html]
+        },
+        {
             'expression':
                 textwrap.dedent("""
                     ${"Mixed {span_start}text{span_end}".format(