Python readability review for dbeam@.

third_party/closure_compiler/checker.py

 #!/usr/bin/python

[rothwell, 2014/12/18 22:30:47]

are there unittests for this file?

[Dan Beam, 2015/02/25 03:36:49]

this file mainly just runs a .jar file.  the majority of trickiness is in
Processor, IMO.

what would you have me test?  the arguments passed to the jar?  maybe the error
formatting/manipulation?

actually running the compiler itself seems more like a functional or integration
test than a unit test (as it'd be testing more than this component).  we do have
those tests, and I'm working on getting them running continuously soon.

-# Copyright 2014 The Chromium Authors. All rights reserved.
+# Copyright 2014 The Chromium Authors. All rights reserved. 
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Runs Closure compiler on a JavaScript file to check for errors."""
 
 import argparse
 import os
 import re
 import subprocess
 import sys
@@ skipping 39 matching lines @@
   ]
 
   _JAR_COMMAND = [
     "java",
     "-jar",
     "-Xms1024m",
     "-client",
     "-XX:+TieredCompilation"
   ]
 
   _found_java = False

[rothwell, 2014/12/18 22:30:47]

why is this a class variable and not an instance variable (comment)

[rothwell, 2014/12/18 22:30:47]

add a comment?

[Dan Beam, 2015/02/25 03:36:49]

I've just removed checking for java on the user's path.  The script can just
crash and burn in this case.

 
   def __init__(self, verbose=False):

[rothwell, 2014/12/18 22:30:47]

no docstring?

[Dan Beam, 2015/02/25 03:36:48]

Done.

     current_dir = os.path.join(os.path.dirname(__file__))
     self._compiler_jar = os.path.join(current_dir, "lib", "compiler.jar")
     self._runner_jar = os.path.join(current_dir, "runner", "runner.jar")
     self._temp_files = []
     self._verbose = verbose
     self._error_filter = error_filter.PromiseErrorFilter()
 
   def _clean_up(self):
     if not self._temp_files:
       return
 
     self._debug("Deleting temporary files: %s" % ", ".join(self._temp_files))
     for f in self._temp_files:
       os.remove(f)
     self._temp_files = []
 
   def _debug(self, msg, error=False):
     if self._verbose:
       print "(INFO) %s" % msg

[rothwell, 2014/12/18 22:30:48]

there's no logging module that you could/should be using here?

[Dan Beam, 2015/02/25 03:36:49]

this CL is getting kinda big.  can I do this separately for you?  fwiw: not sure
how much we use the logging module in chrome.

 
   def _error(self, msg):

[rothwell, 2014/12/18 22:30:47]

maybe this should be called _log_error - I would expect a method named like this
to raise an exception

[Dan Beam, 2015/02/25 03:36:49]

Done.

     print >> sys.stderr, "(ERROR) %s" % msg
     self._clean_up()

[rothwell, 2014/12/18 22:30:47]

this seems like a confusing side effect of this method

[Dan Beam, 2015/02/25 03:36:48]

This originally had a reason to be here, but now I don't see why it should be.

Removed.

 
   def _run_command(self, cmd):
     """Runs a shell command.
 
     Args:
         cmd: A list of tokens to be joined into a shell command.

[rothwell, 2014/12/18 22:30:47]

minor nit but indention here differs from google3 - chromium is different?

[Dan Beam, 2015/02/25 03:36:49]

Done.

 
     Return:
         True if the exit code was 0, else False.

[rothwell, 2014/12/18 22:30:47]

this doesn't look correct - it appears to return the process handle.

[Dan Beam, 2015/02/25 03:36:49]

Done.

     """
     cmd_str = " ".join(cmd)
     self._debug("Running command: %s" % cmd_str)
 
     devnull = open(os.devnull, "w")
     return subprocess.Popen(
         cmd_str, stdout=devnull, stderr=subprocess.PIPE, shell=True)
 
   def _check_java_path(self):

[rothwell, 2014/12/18 22:30:47]

maybe this should be a classmethod as it modifies class state

[Dan Beam, 2015/02/25 03:36:49]

Removed.

     """Checks that `java` is on the system path."""
     if not self._found_java:
       proc = self._run_command(["which", "java"])
       proc.communicate()
       if proc.returncode == 0:
         self._found_java = True
       else:
         self._error("Cannot find java (`which java` => %s)" % proc.returncode)
 
     return self._found_java
 
   def _run_jar(self, jar, args=None):
     args = args or []
     self._check_java_path()
     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.

[rothwell, 2014/12/18 22:30:47]

maybe elaborate on why this conversion is necessary? why would the line numbers
in the tmp file not match the original?

[Dan Beam, 2015/02/25 03:36:48]

Done.

 
     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)
 
@@ skipping 29 matching lines @@
   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)

[rothwell, 2014/12/18 22:30:47]

is exitcode useful to the caller? I would expect a bool to make more sense.

[Dan Beam, 2015/02/25 03:36:49]

Done.

             and its output (as a string).
     """
     depends = depends or []
     externs = externs or set()
 
     if not self._check_java_path():
       return 1, ""
 
     self._debug("FILE: %s" % source_file)
 
     if source_file.endswith("_externs.js"):
       self._debug("Skipping externs: %s" % source_file)
       return

[rothwell, 2014/12/18 22:30:47]

this return code doesn't match your stated API

[Dan Beam, 2015/02/25 03:36:48]

Done.

 
     self._file_arg = source_file
 
     tmp_dir = tempfile.gettempdir()
     rel_path = lambda f: os.path.join(os.path.relpath(os.getcwd(), tmp_dir), f)

[rothwell, 2014/12/18 22:30:47]

not sure this lambda is buying you much, I would just move this into the list
comprehension

[Dan Beam, 2015/02/25 03:36:49]

I find:

  includes = [rel_path(f) for f in depends + [source_file]]

more readable than:

  includes = [os.path.join(os.path.relpath(os.getcwd(), tmp_dir), f)
      for f in depends + [source_file]]

do you disagree?

[rothwell, 2015/03/20 19:24:31]

defer to your preference

 
     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: %s" % meta_file)
 
     self._processor = processor.Processor(meta_file)
     self._expanded_file = self._create_temp_file(self._processor.contents)
     self._debug("Expanded file: %s" % self._expanded_file)
 
@@ skipping 19 matching lines @@
 
     output = self._format_errors(map(self._fix_up_error, errors))
     if errors:
       prefix = "\n" if output else ""
       self._error("Error in: %s%s%s" % (source_file, prefix, output))
     elif output:
       self._debug("Output: %s" % output)
 
     self._clean_up()
 
     return bool(errors), output

[rothwell, 2014/12/18 22:30:47]

your API says you will return an integer, this is a boolean

[Dan Beam, 2015/02/25 03:36:49]

Done.

 
 
 if __name__ == "__main__":
   parser = argparse.ArgumentParser(
       description="Typecheck JavaScript using Closure compiler")
   parser.add_argument("sources", nargs=argparse.ONE_OR_MORE,
                       help="Path to a source file to typecheck")
   parser.add_argument("-d", "--depends", nargs=argparse.ZERO_OR_MORE)
   parser.add_argument("-e", "--externs", nargs=argparse.ZERO_OR_MORE)
   parser.add_argument("-o", "--out_file", help="A place to output results")
   parser.add_argument("-v", "--verbose", action="store_true",
                       help="Show more information as this script runs")
   opts = parser.parse_args()
 
   checker = Checker(verbose=opts.verbose)
   for source in opts.sources:
     depends, externs = build.inputs.resolve_recursive_dependencies(
         source,
         opts.depends,
         opts.externs)
     exit, _ = checker.check(source, depends=depends, externs=externs)

[rothwell, 2014/12/18 22:30:48]

why return an error string if it's just going to be ignored?

[Dan Beam, 2015/02/25 03:36:49]

it is used here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...

but these tests aren't passing anyways, so Done.

     if exit != 0:
       sys.exit(exit)
 
     if opts.out_file:
       out_dir = os.path.dirname(opts.out_file)
       if not os.path.exists(out_dir):
         os.makedirs(out_dir)
       # TODO(dbeam): write compiled file to |opts.out_file|.
       open(opts.out_file, "w").write("")

[rothwell, 2014/12/18 22:30:47]

it seems confusing to write an empty file - shouldn't this feature just be
unimplemented for now, rather than incompletely implemented?

[Dan Beam, 2015/02/25 03:36:49]

this empty file is used by the build system for incremental build purposes (e.g.
if the source file is newer than this empty file, recompile).  added more doc.