Converting various classes to namedtuples

appengine/findit/lib/gitiles/change_log.py

Index: appengine/findit/lib/gitiles/change_log.py
diff --git a/appengine/findit/lib/gitiles/change_log.py b/appengine/findit/lib/gitiles/change_log.py
index b14c93f03b1f6da63bcf21d49bfc353a394d8b8a..3f1686dfe2956c5e3f25445dddd7be55b3c2037e 100644
--- a/appengine/findit/lib/gitiles/change_log.py
+++ b/appengine/findit/lib/gitiles/change_log.py
@@ -2,13 +2,45 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-# TODO(http://crbug.com/661822): convert this into a namedtuple.
-class FileChangeInfo(object):
+from collections import namedtuple
+
+from lib.gitiles.diff import ChangeType
+
+# TODO(wrengr): it'd be better to have a class hierarchy here, so we can
+# avoid playing around with None, and so the change_type can be stored
+# once (in the class itself; rather than once per instance).
+# TODO(http://crbug/644476): better name for this class; i.e., without
+# the extraneous \"Info\" at the very least.
+# TODO(http://crbug.com/659346): coverage tests for the smart constructors.
+class FileChangeInfo(namedtuple('FileChangeInfo',
+    ['change_type', 'old_path', 'new_path'])):
   """Represents a file change (add/delete/modify/rename/copy/etc)."""
-  def __init__(self, change_type, old_path, new_path):
-    self.change_type = change_type
-    self.old_path = old_path
-    self.new_path = new_path
+  __slots__ = ()
+
+  @classmethod
+  def Modify(cls, path): # pragma: no cover
+    return cls(ChangeType.MODIFY, path, path)
+
+  @classmethod
+  def Add(cls, path): # pragma: no cover
+    # Stay the same as gitile.
+    return cls(ChangeType.ADD, None, path)
+
+  @classmethod
+  def Delete(cls, path): # pragma: no cover
+    return cls(ChangeType.DELETE, path, None)
+
+  @classmethod
+  def Rename(cls, old_path, new_path): # pragma: no cover
+    return cls(ChangeType.RENAME, old_path, new_path)
+
+  @classmethod
+  def Copy(cls, old_path, new_path): # pragma: no cover
+    return cls(ChangeType.COPY, old_path, new_path)
+
+  @classmethod
+  def FromDict(cls, info):
+    return cls(info['change_type'].lower(), info['old_path'], info['new_path'])
 
   def ToDict(self):
     return {

[stgao, 2016/11/22 21:22:59]

IIRC, namedtuple has an equivalent func of this, but I can't find it out
immediately. Mind a check?

[wrengr, 2016/11/22 21:32:39]

There's the ``_asdict`` method, which returns an ``OrderedDict``. (Just as one
can use ``MyNamedTuple(**some_dict)`` to implement something like ``FromDict``.)
However I'm a bit wary of using it directly because of semantic mismatch. The
goal of ``_asdict`` is to convert a namedtuple into a Python ``dict`` for
introspection purposes; whereas, the goal of ToDict is to convert the namedtuple
into JSON for sending over the wire. The names of fields in Python need not be
the same as the names of the fields in JSON, and indeed we already have cases
where they differ.

[stgao, 2016/11/22 21:50:39]

Good point. Sgtm!

@@ -17,33 +49,24 @@ class FileChangeInfo(object):
         'new_path': self.new_path
     }
 
-  @staticmethod
-  def FromDict(info):
-    return FileChangeInfo(
-        info['change_type'], info['old_path'], info['new_path'])
-
 
-# TODO(http://crbug.com/661822): convert this into a namedtuple.
-class ChangeLog(object):
+class ChangeLog(namedtuple('ChangeLog',
+    ['author_name', 'author_email', 'author_time', 'committer_name',
+     'committer_email', 'committer_time', 'revision', 'commit_position',
+     'message', 'touched_files', 'commit_url', 'code_review_url',
+     'reverted_revision'])):
   """Represents the change log of a revision."""
+  __slots__ = ()
 
-  def __init__(self, author_name, author_email, author_time, committer_name,
-               committer_email, committer_time, revision, commit_position,
-               message, touched_files, commit_url, code_review_url=None,
-               reverted_revision=None):
-    self.author_name = author_name
-    self.author_email = author_email
-    self.author_time = author_time
-    self.committer_name = committer_name
-    self.committer_email = committer_email
-    self.committer_time = committer_time
-    self.revision = revision
-    self.commit_position = commit_position
-    self.touched_files = touched_files
-    self.message = message
-    self.commit_url = commit_url
-    self.code_review_url = code_review_url
-    self.reverted_revision = reverted_revision
+  def __new__(cls, author_name, author_email, author_time, committer_name,
+              committer_email, committer_time, revision, commit_position,
+              message, touched_files, commit_url, code_review_url=None,
+              reverted_revision=None):
+    return super(cls, ChangeLog).__new__(cls,
+        author_name, author_email, author_time, committer_name,
+        committer_email, committer_time, revision, commit_position,
+        message, touched_files, commit_url, code_review_url, 
+        reverted_revision)
 
   def ToDict(self):
     """Returns the change log as a JSON object."""
@@ -71,7 +94,12 @@ class ChangeLog(object):
     """Returns a ChangeLog instance represented by the given JSON info."""
     touched_files = []
     for touched_file_info in info['touched_files']:
-      touched_files.append(FileChangeInfo.FromDict(touched_file_info))
+      if isinstance(touched_file_info, dict):
+        touched_file_info = FileChangeInfo.FromDict(touched_file_info)
+      if not isinstance(touched_file_info, FileChangeInfo): # pragma: no cover
+        raise TypeError("expected FileChangeInfo but got %s"
+            % touched_file_info.__class__.__name__)
+      touched_files.append(touched_file_info)
 
     return ChangeLog(
         info['author_name'], info['author_email'], info['author_time'],