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

Add ability to format errors as a list of temp-allow rules to paste
into a DEPS file.

The idea behind this change is that the tool now lets you do this:
a) Update a DEPS file for some directory with the intended rules for
the directory, e.g. rules disallowing certain directories (say
"chrome/browser") and allowing others (e.g. "chrome/browser/apis").
b) Run this tool on that particular directory to generate a list of
temp-allow rules.
c) Paste the temp-allow rules into the DEPS file for the directory.
d) Work on eliminating dependencies until the temp-allow rules list in
the DEPS file reaches zero.

To accomplish this change, and fix the bug that existed that the
checkdeps tool didn't work correctly if you specified a start
directory, I refactored the code to always use the approach I had
introduced in CheckAddedCppIncludes, which when you request rules for
a given directory generates all parent directories' rules first, up to
the base directory.

Also, to enable better extraction of error information for the
eventual formatter, I externalized formatting to separate classes
and had the code collect only the logical error information.

While in there, fixed use of quotes to match coding standard.

BUG=138280
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149455

[erikwright (departed), 2012-07-31 17:22:45]

I don't have Python readability but I will brush up on the styleguide and do a
style pass before the final OK.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:249: directory that owns the |parent_rules|, add
the current
'current directory' is a bit confusing here. I assume it means |dir_path|.

'add the current directory's rules' -> 'add |dir_path|'s rules'

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:253: rules_tuple =
self._ApplyDirectoryRules(parent_rules, dir_path)
For documentation, might I suggest something like:

(directory_rules, excluded_subdirs) = self._ApplyDirectoryRules(...)

This would save me having to go up and read the docs for _ApplyDirectoryRules to
understand its return data.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:256: # We skip this one case for running tests.
This comment is confusing and probably unnecessary if you implement my
suggestion re: decomposing the tuple.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:272: if not dir_path.startswith(
shouldn't this be 'if not norm_dir_path.startswith(...):'?

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:310: return self._CheckDirectoryImpl(checkers,
start_dir)
It seems that you need to change this statement to get the strings from the
results_formatter?

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/java_checke...
File tools/checkdeps/java_checker.py (right):

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/java_checke...
tools/checkdeps/java_checker.py:102: results.DependencyViolation(include_path,
rule, rules))
I note that you don't check for 'fail_on_temp_allow' here (vs CPP).

It might be more appropriate to pass each include path and rule to
dependee_status. It could accumulate all temp_allow and disallow (and allow, why
not?) and the client of the Checker can decide what to do with them.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py
File tools/checkdeps/results.py (right):

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:13: # Path of the included or imported file that is
in violation of a rule.
I would write this as "The include or import path that is in violation of a
rule."

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:16: # The specific rule
The violated rule.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:19: # The set of rules containing self.violated_rules
violated_rules -> violated_rule
Add a '.'

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:24: """Results object for a file that has one or more
violations."""
"one or more" seems wrong, given the presence of "HasViolations"

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:27: # Path of the file that has the
dependencies-in-violation listed
Maybe "Path of the file whose nonconforming dependencies are listed in
self.violations."?

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:48: self.results = []
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.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:84: def FormatViolation(self, violation):
I would convert this into a class function taking verbose as an argument.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:101: """A results formatter that produces a single
line per file that
"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.

[Jói, 2012-08-01 11:16:32]

Please take another look.

On Tue, Jul 31, 2012 at 5:22 PM,  <erikwright@chromium.org> wrote:
> I don't have Python readability but I will brush up on the styleguide and do
> a
> style pass before the final OK.
>
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.py
> File tools/checkdeps/checkdeps.py (right):
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
> tools/checkdeps/checkdeps.py:249: directory that owns the
> |parent_rules|, add the current
> 'current directory' is a bit confusing here. I assume it means
> |dir_path|.
>
> 'add the current directory's rules' -> 'add |dir_path|'s rules'
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
> tools/checkdeps/checkdeps.py:253: rules_tuple =
> self._ApplyDirectoryRules(parent_rules, dir_path)
> For documentation, might I suggest something like:
>
> (directory_rules, excluded_subdirs) = self._ApplyDirectoryRules(...)
>
> This would save me having to go up and read the docs for
> _ApplyDirectoryRules to understand its return data.
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
> tools/checkdeps/checkdeps.py:256: # We skip this one case for running
> tests.
> This comment is confusing and probably unnecessary if you implement my
> suggestion re: decomposing the tuple.
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
> tools/checkdeps/checkdeps.py:272: if not dir_path.startswith(
> shouldn't this be 'if not norm_dir_path.startswith(...):'?
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
> tools/checkdeps/checkdeps.py:310: return
> self._CheckDirectoryImpl(checkers, start_dir)
> It seems that you need to change this statement to get the strings from
> the results_formatter?
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/java_checke...
> File tools/checkdeps/java_checker.py (right):
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/java_checke...
> tools/checkdeps/java_checker.py:102:
> results.DependencyViolation(include_path, rule, rules))
> I note that you don't check for 'fail_on_temp_allow' here (vs CPP).
>
> It might be more appropriate to pass each include path and rule to
> dependee_status. It could accumulate all temp_allow and disallow (and
> allow, why not?) and the client of the Checker can decide what to do
> with them.
>
> http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py
> File tools/checkdeps/results.py (right):
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
> tools/checkdeps/results.py:13: # Path of the included or imported file
> that is in violation of a rule.
> I would write this as "The include or import path that is in violation
> of a rule."
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
> tools/checkdeps/results.py:16: # The specific rule
> The violated rule.
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
> tools/checkdeps/results.py:19: # The set of rules containing
> self.violated_rules
> violated_rules -> violated_rule
> Add a '.'
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
> tools/checkdeps/results.py:24: """Results object for a file that has one
> or more violations."""
> "one or more" seems wrong, given the presence of "HasViolations"
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
> tools/checkdeps/results.py:27: # Path of the file that has the
> dependencies-in-violation listed
> Maybe "Path of the file whose nonconforming dependencies are listed in
> self.violations."?
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
> tools/checkdeps/results.py:48: self.results = []
> 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.
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
> tools/checkdeps/results.py:84: def FormatViolation(self, violation):
> I would convert this into a class function taking verbose as an
> argument.
>
>
http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
> tools/checkdeps/results.py:101: """A results formatter that produces a
> single line per file that
> "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.
>
> http://codereview.chromium.org/10832062/

[erikwright (departed), 2012-08-01 14:57:04]

LGTM. Skimmed the Python styleguide and it seems justly applied.

http://codereview.chromium.org/10832062/diff/7002/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10832062/diff/7002/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:364: violation_lines =
results.NormalResultsFormatter.FormatViolation(
consider 'from results import NormalResultsFormatter, TemporaryRulesFormatter'.

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

Thanks, will commit.  I fixed the last nit (from .. import).  There's a bunch of
stuff below because I just saw that I must have done a straight reply rather
than 'Publish+Mail Comments' last time around.

Cheers,
Jói

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:249: directory that owns the |parent_rules|, add
the current
On 2012/07/31 17:22:45, erikwright wrote:
> 'current directory' is a bit confusing here. I assume it means |dir_path|.
> 
> 'add the current directory's rules' -> 'add |dir_path|'s rules'

Done.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:253: rules_tuple =
self._ApplyDirectoryRules(parent_rules, dir_path)
On 2012/07/31 17:22:45, erikwright wrote:
> For documentation, might I suggest something like:
> 
> (directory_rules, excluded_subdirs) = self._ApplyDirectoryRules(...)
> 
> This would save me having to go up and read the docs for _ApplyDirectoryRules
to
> understand its return data.

Done.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:256: # We skip this one case for running tests.
On 2012/07/31 17:22:45, erikwright wrote:
> This comment is confusing and probably unnecessary if you implement my
> suggestion re: decomposing the tuple.

Whoops, it was something I meant to remove along with code that used to live
there.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:272: if not dir_path.startswith(
On 2012/07/31 17:22:45, erikwright wrote:
> shouldn't this be 'if not norm_dir_path.startswith(...):'?

Quite right, fixed.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:310: return self._CheckDirectoryImpl(checkers,
start_dir)
On 2012/07/31 17:22:46, erikwright wrote:
> It seems that you need to change this statement to get the strings from the
> results_formatter?

Done.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/java_checke...
File tools/checkdeps/java_checker.py (right):

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/java_checke...
tools/checkdeps/java_checker.py:102: results.DependencyViolation(include_path,
rule, rules))
On 2012/07/31 17:22:46, erikwright wrote:
> I note that you don't check for 'fail_on_temp_allow' here (vs CPP).
> 
> It might be more appropriate to pass each include path and rule to
> dependee_status. It could accumulate all temp_allow and disallow (and allow,
why
> not?) and the client of the Checker can decide what to do with them.

I might do this as a follow-up change; making the temp-allow stuff work with
Java files is a TODO.  For now, I don't want to mess too much with the possible
performance characteristics of checkdeps.py, especially since I made it a
presubmit check to verify added and modified lines against the DEPS rules.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py
File tools/checkdeps/results.py (right):

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:13: # Path of the included or imported file that is
in violation of a rule.
On 2012/07/31 17:22:46, erikwright wrote:
> I would write this as "The include or import path that is in violation of a
> rule."

Done.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:16: # The specific rule
On 2012/07/31 17:22:46, erikwright wrote:
> The violated rule.

Done.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:19: # The set of rules containing self.violated_rules
On 2012/07/31 17:22:46, erikwright wrote:
> violated_rules -> violated_rule
> Add a '.'

Done.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:24: """Results object for a file that has one or more
violations."""
On 2012/07/31 17:22:46, erikwright wrote:
> "one or more" seems wrong, given the presence of "HasViolations"

Done.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:27: # Path of the file that has the
dependencies-in-violation listed
On 2012/07/31 17:22:46, erikwright wrote:
> Maybe "Path of the file whose nonconforming dependencies are listed in
> self.violations."?

Done.

http://codereview.chromium.org/10832062/diff/2001/tools/checkdeps/results.py#...
tools/checkdeps/results.py:48: self.results = []
On 2012/07/31 17:22:46, erikwright wrote:
> 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.

Done.

http://codereview.chromium.org/10832062/diff/7002/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10832062/diff/7002/tools/checkdeps/checkdeps.p...
tools/checkdeps/checkdeps.py:364: violation_lines =
results.NormalResultsFormatter.FormatViolation(
On 2012/08/01 14:57:04, erikwright wrote:
> consider 'from results import NormalResultsFormatter,
TemporaryRulesFormatter'.

Switched to that approach.

[commit-bot: I haz the power, 2012-08-01 15:23:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10832062/12001

[commit-bot: I haz the power, 2012-08-01 19:02:27]

Change committed as 149455