Auto merge of #11755 - Wafflespeanut:tidy_fixes, r=nox

Removed an unused function and minor cleanup

fixes #11679

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11755)
<!-- Reviewable:end -->

2 files changed, 64 insertions, 77 deletions

diff --git a/python/tidy/servo_tidy/tidy.py b/python/tidy/servo_tidy/tidy.py
index 7f263aa902a..1bd0c528b7e 100644
--- a/python/tidy/servo_tidy/tidy.py
+++ b/python/tidy/servo_tidy/tidy.py
@@ -19,19 +19,21 @@ import subprocess
 import sys
 from licenseck import licenses
 
+# License and header checks
+EMACS_HEADER = "/* -*- Mode:"
+VIM_HEADER = "/* vim:"
+MAX_LICENSE_LINESPAN = max(len(license.splitlines()) for license in licenses)
+
 # File patterns to include in the non-WPT tidy check.
-file_patterns_to_check = ["*.rs", "*.rc", "*.cpp", "*.c",
+FILE_PATTERNS_TO_CHECK = ["*.rs", "*.rc", "*.cpp", "*.c",
                           "*.h", "Cargo.lock", "*.py",
                           "*.toml", "*.webidl", "*.json"]
 
 # File patterns that are ignored for all tidy and lint checks.
-file_patterns_to_ignore = [
-    "*.#*",
-    "*.pyc",
-]
+FILE_PATTERNS_TO_IGNORE = ["*.#*", "*.pyc"]
 
 # Files that are ignored for all tidy and lint checks.
-ignored_files = [
+IGNORED_FILES = [
     # Generated and upstream code combined with our own. Could use cleanup
     os.path.join(".", "ports", "geckolib", "gecko_bindings", "bindings.rs"),
     os.path.join(".", "ports", "geckolib", "gecko_bindings", "structs_debug.rs"),
@@ -49,7 +51,7 @@ ignored_files = [
 ]
 
 # Directories that are ignored for the non-WPT tidy check.
-ignored_dirs = [
+IGNORED_DIRS = [
     # Upstream
     os.path.join(".", "support", "android", "apk"),
     os.path.join(".", "support", "rust-task_info"),
@@ -74,7 +76,31 @@ ignored_dirs = [
     os.path.join(".", "."),
 ]
 
-spec_base_path = "components/script/dom/"
+SPEC_BASE_PATH = "components/script/dom/"
+
+WEBIDL_STANDARDS = [
+    "//www.khronos.org/registry/webgl/specs",
+    "//developer.mozilla.org/en-US/docs/Web/API",
+    "//dev.w3.org/2006/webapi",
+    "//dev.w3.org/csswg",
+    "//dev.w3.org/fxtf",
+    "//dvcs.w3.org/hg",
+    "//dom.spec.whatwg.org",
+    "//domparsing.spec.whatwg.org",
+    "//drafts.csswg.org/cssom",
+    "//drafts.fxtf.org",
+    "//encoding.spec.whatwg.org",
+    "//html.spec.whatwg.org",
+    "//url.spec.whatwg.org",
+    "//xhr.spec.whatwg.org",
+    "//w3c.github.io",
+    "//heycam.github.io/webidl",
+    "//webbluetoothcg.github.io/web-bluetooth/",
+    "//slightlyoff.github.io/ServiceWorker/spec/service_worker/",
+    # Not a URL
+    "// This interface is entirely internal to Servo, and should not be" +
+    " accessible to\n// web pages."
+]
 
 
 def is_iter_empty(iterator):
@@ -97,16 +123,16 @@ def progress_wrapper(iterator):
 
 
 def filter_file(file_name):
-    if any(file_name.startswith(ignored_file) for ignored_file in ignored_files):
+    if any(file_name.startswith(ignored_file) for ignored_file in IGNORED_FILES):
         return False
     base_name = os.path.basename(file_name)
-    if any(fnmatch.fnmatch(base_name, pattern) for pattern in file_patterns_to_ignore):
+    if any(fnmatch.fnmatch(base_name, pattern) for pattern in FILE_PATTERNS_TO_IGNORE):
         return False
     return True
 
 
 def filter_files(start_dir, only_changed_files, progress):
-    file_iter = get_file_list(start_dir, only_changed_files, ignored_dirs)
+    file_iter = get_file_list(start_dir, only_changed_files, IGNORED_DIRS)
     (has_element, file_iter) = is_iter_empty(file_iter)
     if not has_element:
         raise StopIteration
@@ -114,18 +140,13 @@ def filter_files(start_dir, only_changed_files, progress):
         file_iter = progress_wrapper(file_iter)
     for file_name in file_iter:
         base_name = os.path.basename(file_name)
-        if not any(fnmatch.fnmatch(base_name, pattern) for pattern in file_patterns_to_check):
+        if not any(fnmatch.fnmatch(base_name, pattern) for pattern in FILE_PATTERNS_TO_CHECK):
             continue
         if not filter_file(file_name):
             continue
         yield file_name
 
 
-EMACS_HEADER = "/* -*- Mode:"
-VIM_HEADER = "/* vim:"
-MAX_LICENSE_LINESPAN = max(len(license.splitlines()) for license in licenses)
-
-
 def check_license(file_name, lines):
     if any(file_name.endswith(ext) for ext in (".toml", ".lock", ".json")):
         raise StopIteration
@@ -273,13 +294,6 @@ duplicate versions for package "{package}"
                 yield (1, message)
 
 
-def maybe_int(value):
-    try:
-        return int(value)
-    except ValueError:
-        return value
-
-
 def check_toml(file_name, lines):
     if not file_name.endswith(".toml"):
         raise StopIteration
@@ -295,9 +309,9 @@ def check_rust(file_name, lines):
        file_name.endswith(os.path.join("geckolib", "build.rs")) or \
        file_name.endswith(os.path.join("unit", "style", "stylesheets.rs")):
         raise StopIteration
+
     comment_depth = 0
     merged_lines = ''
-
     import_block = False
     whitespace = False
 
@@ -314,6 +328,8 @@ def check_rust(file_name, lines):
     for idx, original_line in enumerate(lines):
         # simplify the analysis
         line = original_line.strip()
+        is_attribute = re.search(r"#\[.*\]", line)
+        is_comment = re.search(r"^//|^/\*|^\*", line)
 
         # Simple heuristic to avoid common case of no comments.
         if '/' in line:
@@ -331,17 +347,17 @@ def check_rust(file_name, lines):
             merged_lines = ''
 
         # Keep track of whitespace to enable checking for a merged import block
-        #
+
         # Ignore attributes, comments, and imports
         if import_block:
-            if not (line_is_comment(line) or line_is_attribute(line) or line.startswith("use ")):
+            if not (is_comment or is_attribute or line.startswith("use ")):
                 whitespace = line == ""
 
                 if not whitespace:
                     import_block = False
 
         # get rid of strings and chars because cases like regex expression, keep attributes
-        if not line_is_attribute(line):
+        if not is_attribute:
             line = re.sub(r'"(\\.|[^\\"])*?"', '""', line)
             line = re.sub(r"'(\\.|[^\\'])*?'", "''", line)
 
@@ -355,21 +371,22 @@ def check_rust(file_name, lines):
         # tuple format: (pattern, format_message, filter_function(match, line))
         no_filter = lambda match, line: True
         regex_rules = [
-            (r",[^\s]", "missing space after ,", lambda match, line: '$' not in line),
+            (r",[^\s]", "missing space after ,",
+                lambda match, line: '$' not in line and not is_attribute),
             (r"[A-Za-z0-9\"]=", "missing space before =",
-                lambda match, line: line_is_attribute(line)),
+                lambda match, line: is_attribute),
             (r"=[A-Za-z0-9\"]", "missing space after =",
-                lambda match, line: line_is_attribute(line)),
+                lambda match, line: is_attribute),
             # ignore scientific notation patterns like 1e-6
             (r"[A-DF-Za-df-z0-9]-", "missing space before -",
-                lambda match, line: not line_is_attribute(line)),
+                lambda match, line: not is_attribute),
             (r"[A-Za-z0-9]([\+/\*%=])", "missing space before {0}",
-                lambda match, line: (not line_is_attribute(line) and
+                lambda match, line: (not is_attribute and
                                      not is_associated_type(match, line))),
             # * not included because of dereferencing and casting
             # - not included because of unary negation
             (r'([\+/\%=])[A-Za-z0-9"]', "missing space after {0}",
-                lambda match, line: (not line_is_attribute(line) and
+                lambda match, line: (not is_attribute and
                                      not is_associated_type(match, line))),
             (r"\)->", "missing space before ->", no_filter),
             (r"->[A-Za-z]", "missing space after ->", no_filter),
@@ -402,10 +419,8 @@ def check_rust(file_name, lines):
 
         for pattern, message, filter_func in regex_rules:
             for match in re.finditer(pattern, line):
-                if not filter_func(match, line):
-                    continue
-
-                yield (idx + 1, message.format(*match.groups(), **match.groupdict()))
+                if filter_func(match, line):
+                    yield (idx + 1, message.format(*match.groups(), **match.groupdict()))
 
         if prev_open_brace and not line:
             yield (idx + 1, "found an empty line following a {")
@@ -481,14 +496,6 @@ def is_associated_type(match, line):
     return generic_open and generic_close
 
 
-def line_is_attribute(line):
-    return re.search(r"#\[.*\]", line)
-
-
-def line_is_comment(line):
-    return re.search(r"^//|^/\*|^\*", line)
-
-
 def check_webidl_spec(file_name, contents):
     # Sorted by this function (in pseudo-Rust). The idea is to group the same
     # organization together.
@@ -508,35 +515,14 @@ def check_webidl_spec(file_name, contents):
     #     }
     #     a_domain.path().cmp(b_domain.path())
     # }
+
     if not file_name.endswith(".webidl"):
         raise StopIteration
-    standards = [
-        "//www.khronos.org/registry/webgl/specs",
-        "//developer.mozilla.org/en-US/docs/Web/API",
-        "//dev.w3.org/2006/webapi",
-        "//dev.w3.org/csswg",
-        "//dev.w3.org/fxtf",
-        "//dvcs.w3.org/hg",
-        "//dom.spec.whatwg.org",
-        "//domparsing.spec.whatwg.org",
-        "//drafts.csswg.org/cssom",
-        "//drafts.fxtf.org",
-        "//encoding.spec.whatwg.org",
-        "//html.spec.whatwg.org",
-        "//url.spec.whatwg.org",
-        "//xhr.spec.whatwg.org",
-        "//w3c.github.io",
-        "//heycam.github.io/webidl",
-        "//webbluetoothcg.github.io/web-bluetooth/",
-        "//slightlyoff.github.io/ServiceWorker/spec/service_worker/",
-        # Not a URL
-        "// This interface is entirely internal to Servo, and should not be" +
-        " accessible to\n// web pages."
-    ]
-    for i in standards:
+
+    for i in WEBIDL_STANDARDS:
         if contents.find(i) != -1:
             raise StopIteration
-    yield 0, "No specification link found."
+    yield (0, "No specification link found.")
 
 
 def check_json(filename, contents):
@@ -552,9 +538,9 @@ def check_json(filename, contents):
 
 
 def check_spec(file_name, lines):
-    if spec_base_path not in file_name:
+    if SPEC_BASE_PATH not in file_name:
         raise StopIteration
-    file_name = os.path.relpath(os.path.splitext(file_name)[0], spec_base_path)
+    file_name = os.path.relpath(os.path.splitext(file_name)[0], SPEC_BASE_PATH)
     patt = re.compile("^\s*\/\/.+")
 
     # Pattern representing a line with a macro
@@ -566,9 +552,10 @@ def check_spec(file_name, lines):
     # Pattern representing a line with comment
     comment_patt = re.compile("^\s*///?.+$")
 
-    pattern = "impl {}Methods for {} {{".format(file_name, file_name)
     brace_count = 0
     in_impl = False
+    pattern = "impl {}Methods for {} {{".format(file_name, file_name)
+
     for idx, line in enumerate(lines):
         if "// check-tidy: no specs after this line" in line:
             break
@@ -599,10 +586,10 @@ def collect_errors_for_files(files_to_check, checking_functions, line_checking_f
         raise StopIteration
     if print_text:
         print '\rChecking files for tidiness...'
+
     for filename in files_to_check:
         if not os.path.exists(filename):
             continue
-
         with open(filename, "r") as f:
             contents = f.read()
             for check in checking_functions:
@@ -650,7 +637,7 @@ def get_file_list(directory, only_changed_files=False, exclude_dirs=[]):
         args = ["git", "ls-files", "--others", "--exclude-standard", directory]
         file_list += subprocess.check_output(args)
         for f in file_list.splitlines():
-            if os.path.join('.', os.path.dirname(f)) not in ignored_dirs:
+            if os.path.join('.', os.path.dirname(f)) not in exclude_dirs:
                 yield os.path.join('.', f)
     elif exclude_dirs:
         for root, dirs, files in os.walk(directory, topdown=True):
diff --git a/python/tidy/servo_tidy_tests/test_tidy.py b/python/tidy/servo_tidy_tests/test_tidy.py
index 4b271ce993c..dbbc77c9e23 100644
--- a/python/tidy/servo_tidy_tests/test_tidy.py
+++ b/python/tidy/servo_tidy_tests/test_tidy.py
@@ -79,7 +79,7 @@ class CheckTidiness(unittest.TestCase):
         self.assertNoMoreErrors(errors)
 
     def test_spec_link(self):
-        tidy.spec_base_path = base_path
+        tidy.SPEC_BASE_PATH = base_path
         errors = tidy.collect_errors_for_files(iterFile('speclink.rs'), [], [tidy.check_spec], print_text=False)
         self.assertEqual('method declared in webidl is missing a comment with a specification link', errors.next()[2])
         self.assertNoMoreErrors(errors)