Closure compiler: add pydoc for parts of third_party/closure_compiler/.

third_party/closure_compiler/checker.py

Index: third_party/closure_compiler/checker.py
diff --git a/third_party/closure_compiler/checker.py b/third_party/closure_compiler/checker.py
index e5d776fd4d01c1480f6b40d97db182c2cf90d581..8632bcd1bb68884ee3073a58d3f9318c367cf897 100755
--- a/third_party/closure_compiler/checker.py
+++ b/third_party/closure_compiler/checker.py
@@ -3,17 +3,22 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+"""A script to run Closure compiler on JavaScript file and check for errors."""

[Tyler Breisacher (Chromium), 2014/08/13 19:59:42]

nit: "Runs Closure Compiler on ..."

"A script to" is redundant :)

Likewise with "A class to" below.

[Dan Beam, 2014/08/13 20:23:45]

Done.

+
 import argparse
 import os
-import processor
 import re
 import subprocess
 import sys
 import tempfile
+import processor

[Tyler Breisacher (Chromium), 2014/08/13 19:59:42]

These are generally supposed to be alphabetical right? Why not in this case?

[Dan Beam, 2014/08/13 20:23:45]

processor is not a system import

 
 
 class Checker(object):
-  _common_closure_args = [
+  """A class to run the Closure compiler .jar on a given source file and return
+  the success/errors."""
+
+  _COMMON_CLOSURE_ARGS = [
     "--accept_const_keyword",
     "--language_in=ECMASCRIPT5",
     "--summary_detail_level=3",
@@ -42,9 +47,7 @@ class Checker(object):
     "--jscomp_off=duplicate",
   ]
 
-  _found_java = False
-
-  _jar_command = [
+  _JAR_COMMAND = [
     "java",
     "-jar",
     "-Xms1024m",
@@ -52,6 +55,8 @@ class Checker(object):
     "-XX:+TieredCompilation"
   ]
 
+  _found_java = False

[Tyler Breisacher (Chromium), 2014/08/13 19:59:42]

If I remember how Python works (which is a big if), this is a class property
(Checker._found_java) rather than a property of Checker instances (c =
Checker(); c._found_java) and you need to move it into __init__ to make it an
instance property.

[Dan Beam, 2014/08/13 20:23:45]

i don't want it to be an instance property

[Tyler Breisacher (Chromium), 2014/08/13 20:27:01]

It looks like it's being used that way at line 102, unless I'm misreading it.

+
   def __init__(self, verbose=False):
     current_dir = os.path.join(os.path.dirname(__file__))
     self._compiler_jar = os.path.join(current_dir, "lib", "compiler.jar")
@@ -77,6 +82,14 @@ class Checker(object):
     self._clean_up()
 
   def _run_command(self, cmd):
+    """Runs a shell command.
+
+    Args:
+        cmd: A list of tokens to be joined into a shell command.
+
+    Return:
+        True if the exit code was 0, else False.
+    """
     cmd_str = " ".join(cmd)
     self._debug("Running command: " + cmd_str)
 
@@ -85,6 +98,7 @@ class Checker(object):
         cmd_str, stdout=devnull, stderr=subprocess.PIPE, shell=True)
 
   def _check_java_path(self):
+    """Checks that `java` is on the system path."""
     if not self._found_java:
       proc = self._run_command(["which", "java"])
       proc.communicate()
@@ -95,24 +109,42 @@ class Checker(object):
 
     return self._found_java
 
-  def _run_jar(self, jar, args=[]):
+  def _run_jar(self, jar, args=None):
+    args = args or []
     self._check_java_path()
-    return self._run_command(self._jar_command + [jar] + args)
+    return self._run_command(self._JAR_COMMAND + [jar] + args)
 
   def _fix_line_number(self, match):
+    """Changes a line number from /tmp/file:300 to /orig/file:100.
+
+    Args:
+        match: A re.MatchObject from matching against a line number regex.
+
+    Returns:
+        The fixed up /file and :line number.
+    """
     real_file = self._processor.get_file_from_line(match.group(1))
     return "%s:%d" % (os.path.abspath(real_file.file), real_file.line_number)
 
   def _fix_up_error(self, error):
+    """Filter out irrelevant errors or fix line numbers.
+    
+    Args:
+        error: An Closure compiler error (2 line string with error and source).

[Tyler Breisacher (Chromium), 2014/08/13 19:59:41]

s/An/A/

[Dan Beam, 2014/08/13 20:23:45]

Done.

+    
+    Return:
+        The fixed up erorr string (blank if it should be ignored).
+    """
     if " first declared in " in error:
       # Ignore "Variable x first declared in /same/file".
       return ""
 
-    file = self._expanded_file
-    fixed = re.sub("%s:(\d+)" % file, self._fix_line_number, error)
-    return fixed.replace(file, os.path.abspath(self._file_arg))
+    expanded_file = self._expanded_file
+    fixed = re.sub("%s:(\d+)" % expanded_file, self._fix_line_number, error)
+    return fixed.replace(expanded_file, os.path.abspath(self._file_arg))
 
   def _format_errors(self, errors):
+    """Formats Closure compiler errors to easily spot compiler output."""
     errors = filter(None, errors)
     contents = ("\n" + "## ").join("\n\n".join(errors).splitlines())
     return "## " + contents if contents else ""
@@ -123,22 +155,38 @@ class Checker(object):
       tmp_file.write(contents)
     return tmp_file.name
 
-  def check(self, file, depends=[], externs=[]):
+  def check(self, source_file, depends=None, externs=None):
+    """Closure compile a file and check for errors.
+
+    Args:
+        source_file: A file to check.
+        depends: Other files that would be included with a <script> earlier in
+            the page.
+        externs: @extern files that inform the compiler about custom globals.
+
+    Returns:
+        (exitcode, output) The exit code of the Closure compiler (as a number)
+            and its output (as a string).
+    """
+    depends = depends or []
+    externs = externs or []
+
     if not self._check_java_path():
       return 1, ""
 
-    self._debug("FILE: " + file)
+    self._debug("FILE: " + source_file)
 
-    if file.endswith("_externs.js"):
-      self._debug("Skipping externs: " + file)
+    if source_file.endswith("_externs.js"):
+      self._debug("Skipping externs: " + source_file)
       return
 
-    self._file_arg = file
+    self._file_arg = source_file
 
     tmp_dir = tempfile.gettempdir()
     rel_path = lambda f: os.path.join(os.path.relpath(os.getcwd(), tmp_dir), f)
 
-    contents = ['<include src="%s">' % rel_path(f) for f in depends + [file]]
+    includes = [rel_path(f) for f in depends + [source_file]]
+    contents = ['<include src="%s">' % i for i in includes]
     meta_file = self._create_temp_file("\n".join(contents))
     self._debug("Meta file: " + meta_file)
 
@@ -147,7 +195,7 @@ class Checker(object):
     self._debug("Expanded file: " + self._expanded_file)
 
     args = ["--js=" + self._expanded_file] + ["--externs=" + e for e in externs]
-    args_file_content = " " + " ".join(self._common_closure_args + args)
+    args_file_content = " " + " ".join(self._COMMON_CLOSURE_ARGS + args)
     self._debug("Args: " + args_file_content.strip())
 
     args_file = self._create_temp_file(args_file_content)
@@ -162,7 +210,7 @@ class Checker(object):
 
     output = self._format_errors(map(self._fix_up_error, errors))
     if runner_cmd.returncode:
-      self._error("Error in: " + file + ("\n" + output if output else ""))
+      self._error("Error in: " + source_file + ("\n" + output if output else ""))
     elif output:
       self._debug("Output: " + output)