Auto merge of #12877 - UK992:tidy-toml, r=Wafflespeanut

Tidy config file

This is wip workaround for https://github.com/servo/servo/issues/10841
Adds ``servo-tidy.toml`` with configs and ignored dirs, files and packages.
This will allow to set custom configuration per repo.
It's an example how could config file looks like.
I want opinion on that, if this is right approaches and how to improve it.

cc @edunham

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

python/tidy/servo_tidy/tidy.py

@@ -19,6 +19,25 @@ import subprocess
 import sys
 from licenseck import MPL, APACHE, COPYRIGHT, licenses_toml, licenses_dep_toml
 import colorama
+import toml
+
+CONFIG_FILE_PATH = os.path.join(".", "servo-tidy.toml")
+
+# Default configs
+config = {
+    "skip-check-length": False,
+    "skip-check-licenses": False,
+    "ignore": {
+        "files": [
+            CONFIG_FILE_PATH,  # ignore config file
+            "./.",  # ignore hidden files
+        ],
+        "directories": [
+            "./.",  # ignore hidden directories
+        ],
+        "packages": [],
+    }
+}
 
 COMMENTS = ["// ", "# ", " *", "/* "]
 
@@ -30,53 +49,6 @@ FILE_PATTERNS_TO_CHECK = ["*.rs", "*.rc", "*.cpp", "*.c",
 # File patterns that are ignored for all tidy and lint checks.
 FILE_PATTERNS_TO_IGNORE = ["*.#*", "*.pyc"]
 
-# Files that are ignored for all tidy and lint checks.
-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"),
-    os.path.join(".", "ports", "geckolib", "gecko_bindings", "structs_release.rs"),
-    os.path.join(".", "ports", "geckolib", "string_cache", "atom_macro.rs"),
-    os.path.join(".", "resources", "hsts_preload.json"),
-    os.path.join(".", "tests", "wpt", "metadata", "MANIFEST.json"),
-    os.path.join(".", "tests", "wpt", "metadata-css", "MANIFEST.json"),
-    os.path.join(".", "components", "script", "dom", "webidls", "ForceTouchEvent.webidl"),
-    os.path.join(".", "support", "android", "openssl.sh"),
-    # Ignore those files since the issues reported are on purpose
-    os.path.join(".", "tests", "html", "bad-line-ends.html"),
-    os.path.join(".", "tests", "unit", "net", "parsable_mime", "text"),
-    os.path.join(".", "tests", "wpt", "mozilla", "tests", "css", "fonts"),
-    os.path.join(".", "tests", "wpt", "mozilla", "tests", "css", "pre_with_tab.html"),
-    # FIXME(pcwalton, #11679): This is a workaround for a tidy error on the quoted string
-    # `"__TEXT,_info_plist"` inside an attribute.
-    os.path.join(".", "components", "servo", "platform", "macos", "mod.rs"),
-    # Hidden files
-    os.path.join(".", "."),
-]
-
-# Directories that are ignored for the non-WPT tidy check.
-IGNORED_DIRS = [
-    # Upstream
-    os.path.join(".", "support", "android", "apk"),
-    os.path.join(".", "tests", "wpt", "css-tests"),
-    os.path.join(".", "tests", "wpt", "harness"),
-    os.path.join(".", "tests", "wpt", "update"),
-    os.path.join(".", "tests", "wpt", "web-platform-tests"),
-    os.path.join(".", "tests", "wpt", "mozilla", "tests", "mozilla", "referrer-policy"),
-    os.path.join(".", "tests", "wpt", "sync"),
-    os.path.join(".", "tests", "wpt", "sync_css"),
-    os.path.join(".", "python", "mach"),
-    os.path.join(".", "python", "tidy", "servo_tidy_tests"),
-    os.path.join(".", "components", "script", "dom", "bindings", "codegen", "parser"),
-    os.path.join(".", "components", "script", "dom", "bindings", "codegen", "ply"),
-    os.path.join(".", "python", "_virtualenv"),
-    # Generated and upstream code combined with our own. Could use cleanup
-    os.path.join(".", "target"),
-    os.path.join(".", "ports", "cef"),
-    # Hidden directories
-    os.path.join(".", "."),
-]
-
 SPEC_BASE_PATH = "components/script/dom/"
 
 WEBIDL_STANDARDS = [
@@ -125,7 +97,7 @@ 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 config["ignore"]["files"]):
         return False
     base_name = os.path.basename(file_name)
     if any(fnmatch.fnmatch(base_name, pattern) for pattern in FILE_PATTERNS_TO_IGNORE):
@@ -134,7 +106,7 @@ def filter_file(file_name):
 
 
 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, config["ignore"]["directories"])
     (has_element, file_iter) = is_iter_empty(file_iter)
     if not has_element:
         raise StopIteration
@@ -167,7 +139,8 @@ def licensed_apache(header):
 
 
 def check_license(file_name, lines):
-    if any(file_name.endswith(ext) for ext in (".toml", ".lock", ".json", ".html")):
+    if any(file_name.endswith(ext) for ext in (".toml", ".lock", ".json", ".html")) or \
+       config["skip-check-licenses"]:
         raise StopIteration
 
     if lines[0].startswith("#!") and lines[1].strip():
@@ -202,9 +175,10 @@ def check_modeline(file_name, lines):
 
 
 def check_length(file_name, idx, line):
-    for suffix in [".lock", ".json", ".html", ".toml"]:
+    if any(file_name.endswith(ext) for ext in (".lock", ".json", ".html", ".toml")) or \
-        if file_name.endswith(suffix):
+       config["skip-check-length"]:
-            raise StopIteration
+        raise StopIteration
+
     # Prefer shorter lines when shell scripting.
     if file_name.endswith(".sh"):
         max_length = 80
@@ -296,14 +270,13 @@ def check_lock(file_name, contents):
         raise StopIteration
 
     # package names to be neglected (as named by cargo)
-    exceptions = ["lazy_static"]
+    exceptions = config["ignore"]["packages"]
 
     # toml.py has a bug(?) that we trip up in [metadata] sections;
     # see https://github.com/uiri/toml/issues/61
     # This should only affect a very few lines (that have embedded ?branch=...),
     # and most of them won't be in the repo
     try:
-        import toml
         content = toml.loads(contents)
     except:
         print "WARNING!"
@@ -694,6 +667,79 @@ def check_spec(file_name, lines):
                 brace_count -= 1
 
 
+def check_config_file(config_file, print_text=True):
+    # Check if config file exists
+    if not os.path.exists(config_file):
+        print("%s config file is required but was not found" % config_file)
+        sys.exit(1)
+
+    # Load configs from servo-tidy.toml
+    with open(config_file) as content:
+        conf_file = content.read()
+        lines = conf_file.splitlines(True)
+
+    if print_text:
+        print '\rChecking for config file...'
+
+    current_table = ""
+    for idx, line in enumerate(lines):
+        # Ignore comment lines
+        if line.strip().startswith("#"):
+            continue
+
+        # Check for invalid tables
+        if re.match("\[(.*?)\]", line.strip()):
+            table_name = re.findall(r"\[(.*?)\]", line)[0].strip()
+            if table_name not in ("configs", "ignore"):
+                yield config_file, idx + 1, "invalid config table [%s]" % table_name
+            current_table = table_name
+            continue
+
+        # Skip if there is no equal sign in line, assuming it's not a key
+        if "=" not in line:
+            continue
+
+        key = line.split("=")
+        key = key[0].strip()
+
+        # Check for invalid keys inside [configs] and [ignore] table
+        if (current_table == "configs" and key not in config or
+                current_table == "ignore" and key not in config["ignore"] or
+                # Any key outside of tables
+                current_table == ""):
+            yield config_file, idx + 1, "invalid config key '%s'" % key
+
+    # Parse config file
+    parse_config(conf_file)
+
+
+def parse_config(content):
+    config_file = toml.loads(content)
+    exclude = config_file.get("ignore", {})
+    # Add list of ignored directories to config
+    config["ignore"]["directories"] += exclude.get("directories", [])
+    # Add list of ignored files to config
+    config["ignore"]["files"] += exclude.get("files", [])
+    # Add list of ignored packages to config
+    config["ignore"]["packages"] = exclude.get("packages", [])
+    # Fix the necessary paths if we're in Windows
+    if sys.platform == "win32":
+        files = []
+        for f in config["ignore"]["files"]:
+            files += [os.path.join(*f.split("/"))]
+        config["ignore"]["files"] = files
+        dirs = []
+        for f in config["ignore"]["directories"]:
+            dirs += [os.path.join(*f.split("/"))]
+        config["ignore"]["directories"] = dirs
+
+    # Override default configs
+    configs = config_file.get("configs", [])
+    for pref in configs:
+        if pref in config:
+            config[pref] = configs[pref]
+
+
 def collect_errors_for_files(files_to_check, checking_functions, line_checking_functions, print_text=True):
     (has_element, files_to_check) = is_iter_empty(files_to_check)
     if not has_element:
@@ -773,6 +819,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():
+            f = os.path.join(*f.split("/")) if sys.platform == "win32" else f
             if not any(os.path.join('.', os.path.dirname(f)).startswith(path) for path in exclude_dirs):
                 yield os.path.join('.', f)
     elif exclude_dirs:
@@ -788,6 +835,8 @@ def get_file_list(directory, only_changed_files=False, exclude_dirs=[]):
 
 
 def scan(only_changed_files=False, progress=True):
+    # check config file for errors
+    config_errors = check_config_file(CONFIG_FILE_PATH, progress)
     # standard checks
     files_to_check = filter_files('.', only_changed_files, progress)
     checking_functions = (check_flake8, check_lock, check_webidl_spec, check_json)
@@ -799,7 +848,7 @@ def scan(only_changed_files=False, progress=True):
     # wpt lint checks
     wpt_lint_errors = check_wpt_lint_errors(get_wpt_files(only_changed_files, progress))
     # collect errors
-    errors = itertools.chain(errors, dep_license_errors, wpt_lint_errors)
+    errors = itertools.chain(config_errors, errors, dep_license_errors, wpt_lint_errors)
     error = None
     for error in errors:
         colorama.init()

python/tidy/servo_tidy_tests/servo-tidy.toml

@@ -0,0 +1,13 @@
+key-outside = ""
+
+[configs]
+skip-check-length = false
+skip-check-licenses = false
+wrong-key = false
+
+[wrong]
+wrong-key = true
+
+[ignore]
+files = []
+directories = []

python/tidy/servo_tidy_tests/test_tidy.py

@@ -23,6 +23,13 @@ class CheckTidiness(unittest.TestCase):
         with self.assertRaises(StopIteration):
             errors.next()
 
+    def test_tidy_config(self):
+        errors = tidy.check_config_file(os.path.join(base_path, 'servo-tidy.toml'))
+        self.assertEqual('invalid config key \'key-outside\'', errors.next()[2])
+        self.assertEqual('invalid config key \'wrong-key\'', errors.next()[2])
+        self.assertEqual('invalid config table [wrong]', errors.next()[2])
+        self.assertNoMoreErrors(errors)
+
     def test_spaces_correctnes(self):
         errors = tidy.collect_errors_for_files(iterFile('wrong_space.rs'), [], [tidy.check_by_line], print_text=False)
         self.assertEqual('trailing whitespace', errors.next()[2])

servo-tidy.toml

@@ -0,0 +1,48 @@
+[configs]
+skip-check-length = false
+skip-check-licenses = false
+
+[ignore]
+# Ignored packages with duplicated versions
+packages = ["lazy_static"]
+# Files that are ignored for all tidy and lint checks.
+files = [
+  # Generated and upstream code combined with our own. Could use cleanup
+  "./ports/geckolib/gecko_bindings/bindings.rs",
+  "./ports/geckolib/gecko_bindings/structs_debug.rs",
+  "./ports/geckolib/gecko_bindings/structs_release.rs",
+  "./ports/geckolib/string_cache/atom_macro.rs",
+  "./resources/hsts_preload.json",
+  "./tests/wpt/metadata/MANIFEST.json",
+  "./tests/wpt/metadata-css/MANIFEST.json",
+  "./components/script/dom/webidls/ForceTouchEvent.webidl",
+  "./support/android/openssl.sh",
+  # Ignore those files since the issues reported are on purpose
+  "./tests/html/bad-line-ends.html",
+  "./tests/unit/net/parsable_mime/text",
+  "./tests/wpt/mozilla/tests/css/fonts",
+  "./tests/wpt/mozilla/tests/css/pre_with_tab.html",
+  # FIXME(pcwalton, #11679): This is a workaround for a tidy error on the quoted string
+  # `"__TEXT,_info_plist"` inside an attribute.
+  "./components/servo/platform/macos/mod.rs",
+]
+# Directories that are ignored for the non-WPT tidy check.
+directories = [
+  # Upstream
+  "./support/android/apk",
+  "./tests/wpt/css-tests",
+  "./tests/wpt/harness",
+  "./tests/wpt/update",
+  "./tests/wpt/web-platform-tests",
+  "./tests/wpt/mozilla/tests/mozilla/referrer-policy",
+  "./tests/wpt/sync",
+  "./tests/wpt/sync_css",
+  "./python/mach",
+  "./python/tidy/servo_tidy_tests",
+  "./components/script/dom/bindings/codegen/parser",
+  "./components/script/dom/bindings/codegen/ply",
+  "./python/_virtualenv",
+  # Generated and upstream code combined with our own. Could use cleanup
+  "./target",
+  "./ports/cef",
+]