Add ability to format errors as a list of temp-allow rules to paste

tools/checkdeps/results.py

Index: tools/checkdeps/results.py
diff --git a/tools/checkdeps/results.py b/tools/checkdeps/results.py
new file mode 100644
index 0000000000000000000000000000000000000000..f5018147c89986966aee756894f15edbe1f984e8
--- /dev/null
+++ b/tools/checkdeps/results.py
@@ -0,0 +1,118 @@
+# Copyright (c) 2012 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.
+
+
+"""Results object and results formatters for checkdeps tool."""
+
+
+class DependencyViolation(object):
+  """A single dependency violation."""
+
+  def __init__(self, include_path, violated_rule, rules):
+    # Path of the included or imported file that is in violation of a rule.

[erikwright (departed), 2012/07/31 17:22:46]

I would write this as "The include or import path that is in violation of a
rule."

[Jói, 2012/08/01 15:22:58]

Done.

+    self.include_path = include_path
+
+    # The specific rule

[erikwright (departed), 2012/07/31 17:22:46]

The violated rule.

[Jói, 2012/08/01 15:22:58]

Done.

+    self.violated_rule = violated_rule
+
+    # The set of rules containing self.violated_rules

[erikwright (departed), 2012/07/31 17:22:46]

violated_rules -> violated_rule
Add a '.'

[Jói, 2012/08/01 15:22:58]

Done.

+    self.rules = rules
+
+
+class DependeeStatus(object):
+  """Results object for a file that has one or more violations."""

[erikwright (departed), 2012/07/31 17:22:46]

"one or more" seems wrong, given the presence of "HasViolations"

[Jói, 2012/08/01 15:22:58]

Done.

+
+  def __init__(self, dependee_path):
+    # Path of the file that has the dependencies-in-violation listed

[erikwright (departed), 2012/07/31 17:22:46]

Maybe "Path of the file whose nonconforming dependencies are listed in
self.violations."?

[Jói, 2012/08/01 15:22:58]

Done.

+    # in self.violations.
+    self.dependee_path = dependee_path
+
+    # List of DependencyViolation objects that apply to the dependee
+    # file.
+    self.violations = []
+
+  def AddViolation(self, violation):
+    """Adds a violation."""
+    self.violations.append(violation)
+
+  def HasViolations(self):
+    """Returns True if this dependee is violating one or more rules."""
+    return not not self.violations
+
+
+class ResultsFormatter(object):
+  """Base class for results formatters."""
+
+  def __init__(self):
+    self.results = []

[erikwright (departed), 2012/07/31 17:22:46]

I would pull results into NormalResultsFormatter, since:

1) it's the sole user.
2) it's a bit of a bad smell to be inherited but unused by
TemporaryRulesFormatter.
3) it's hardly sufficiently complex to warrant documentation/re-use if it were
actually re-used.
4) if it were worth re-using, a new base class (between NormalResultsFormatter
and ResultsFormatter) could be introduced.
5) ResultsFormatter becomes a pure interface, which it otherwise resembles.

[Jói, 2012/08/01 15:22:58]

Done.

+
+  def AddError(self, dependee_status):
+    """Add a formatted result to |self.results| for |dependee_status|,
+    which is guaranteed to return True for
+    |dependee_status.HasViolations|.
+    """
+    raise NotImplementedError()
+
+  def GetResults(self):
+    """Returns the results.  May be overridden e.g. to process the
+    results that have been accumulated.
+    """
+    return self.results
+
+  def PrintResults(self):
+    """Prints the results to stdout."""
+    raise NotImplementedError()
+
+
+class NormalResultsFormatter(ResultsFormatter):
+  """A results formatting object that produces the classical,
+  detailed, human-readable output of the checkdeps tool.
+  """
+
+  def __init__(self, verbose):
+    super(NormalResultsFormatter, self).__init__()
+    self.verbose = verbose
+
+  def AddError(self, dependee_status):
+    lines = []
+    lines.append('\nERROR in %s' % dependee_status.dependee_path)
+    for violation in dependee_status.violations:
+      lines.append(self.FormatViolation(violation))
+    self.results.append('\n'.join(lines))
+
+  def FormatViolation(self, violation):

[erikwright (departed), 2012/07/31 17:22:46]

I would convert this into a class function taking verbose as an argument.

+    lines = []
+    if self.verbose:
+      lines.append('For %s' % violation.rules)
+    lines.append(
+        '  Illegal include: "%s"\n    Because of %s' %
+        (violation.include_path, str(violation.violated_rule)))
+    return '\n'.join(lines)
+
+  def PrintResults(self):
+    for result in self.results:
+      print result
+    if self.results:
+      print '\nFAILED\n'
+
+
+class TemporaryRulesFormatter(ResultsFormatter):
+  """A results formatter that produces a single line per file that

[erikwright (departed), 2012/07/31 17:22:46]

"a single line per file that fails checks" indicates to me that a file with
multiple nonconforming includes would produce a single output line.

"A results formatter that produces a single line per nonconforming include. The
combined output is suitable..." works for me.

+  fails checks, that is suitable for directly pasting into a DEPS file
+  as a temporary-allow rule.
+  """
+
+  def __init__(self):
+    self.violations = set()
+
+  def AddError(self, dependee_status):
+    for violation in dependee_status.violations:
+      self.violations.add(violation.include_path)
+
+  def GetResults(self):
+    return ['  "!%s",' % path for path in sorted(self.violations)]
+
+  def PrintResults(self):
+    for result in self.GetResults():
+      print result