Python readability review for dbeam@.

third_party/closure_compiler/processor.py

-# 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.
 
 """Process Chrome resources (HTML/CSS/JS) to handle <include> and <if> tags."""
 
 from collections import defaultdict
 import re
 import os
 
 
 class LineNumber(object):
   """A simple wrapper to hold line information (e.g. file.js:32).
   
   Args:
     source_file: A file path.
     line_number: The line in |file|.
   """
   def __init__(self, source_file, line_number):
     self.file = source_file
     self.line_number = int(line_number)
 
 
 class FileCache(object):

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

It doesn't look like this needs to be a class, it can just be a function.

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

what would I do with _cache?  make it a global?  I'd prefer it to be private to
a class that cares about it.

   """An in-memory cache to speed up reading the same files over and over.
   
   Usage:
       FileCache.read(path_to_file)
   """
 
   _cache = defaultdict(str)
 
   @classmethod
   def read(self, source_file):
@@ skipping 33 matching lines @@
       source_file: A file to process.
 
   Attributes:
       contents: Expanded contents after inlining <include>s and stripping <if>s.
       included_files: A list of files that were inlined via <include>.
   """
 
   _IF_TAGS_REG = "</?if[^>]*?>"
   _INCLUDE_REG = "<include[^>]+src=['\"]([^>]*)['\"]>"
 
   def __init__(self, source_file):

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

docstring - e.g. what is a source_file, a filename or a file object

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

Done.

     self._included_files = set()
     self._index = 0
     self._lines = self._get_file(source_file)
 
     while self._index < len(self._lines):

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

I think you want to be using enumerate here, or document why you can't

       current_line = self._lines[self._index]
       match = re.search(self._INCLUDE_REG, current_line[2])
       if match:
         file_dir = os.path.dirname(current_line[0])
         file_name = os.path.abspath(os.path.join(file_dir, match.group(1)))
         if file_name not in self._included_files:
           self._include_file(file_name)
           continue  # Stay on the same line.

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

^ this is why I can't use enumerate(), because a line needs to be re-processed
after it's changed

         else:
           # Found a duplicate <include>. Ignore and insert a blank line to
           # preserve line numbers.
           self._lines[self._index] = self._lines[self._index][:2] + ("",)
       self._index += 1
 
     for i, line in enumerate(self._lines):
       self._lines[i] = line[:2] + (re.sub(self._IF_TAGS_REG, "", line[2]),)
 
     self.contents = "\n".join(l[2] for l in self._lines)
@@ skipping 11 matching lines @@
   def get_file_from_line(self, line_number):
     """Get the original file and line number for an expanded file's line number.
 
     Args:
         line_number: A processed file's line number.
     """
     line_number = int(line_number) - 1
     return LineNumber(self._lines[line_number][0], self._lines[line_number][1])
 
   @property
   def included_files(self):

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

if you're trying to obscure access to the underlying data structure this should
return a copy of the list. otherwise you should just make included_files a
public field

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

Done. (pydoc doesn't show these properties for me now, though...)

     """A list of files that were inlined via <include>."""
     return self._included_files