diff --git a/scripts/safe_template_linter.py b/scripts/safe_template_linter.py index 2865276b6f..dcb5ff10f7 100755 --- a/scripts/safe_template_linter.py +++ b/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.*?)%>', 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) diff --git a/scripts/safelint_thresholds.json b/scripts/safelint_thresholds.json index cb52c041a1..5fe22e3bda 100644 --- a/scripts/safelint_thresholds.json +++ b/scripts/safelint_thresholds.json @@ -1,32 +1,32 @@ { "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 } diff --git a/scripts/tests/test_safe_template_linter.py b/scripts/tests/test_safe_template_linter.py index 1b788a890d..01e744ed54 100644 --- a/scripts/tests/test_safe_template_linter.py +++ b/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 @@ -358,6 +368,22 @@ class TestMakoTemplateLinter(TestLinter): self.assertEqual(results.violations[0].rule, Rules.python_deprecated_display_name) @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 = '' + _("your course summary page") + '' + 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("""