Detect transformer dependency cycles in packages using barback transformers.

Detect transformer dependency cycles in packages using barback transformers.

R=rnystrom@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=27355

[Bob Nystrom, 2013-09-10 20:31:09]

Some nits then LGTM.

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:67: AssetId
libraryIdentifierToId(String identifier) {
Throw an ArgumentError on an empty string.

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:77: final _libraryPathRegexp = new
RegExp(r"^lib/(.*)\.dart$");
Capitalize "e" in "exp". (I wish the class was named Regex, but we should be
consistent with it.)

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:125: //
multiple cycles we choose which to print deterministically.
"deterministically" -> "consistently".

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:127: in
ordered(graph.packages.values.map((package) => package.name))) {
Instead of wrapping this here, how about hoisting the iterator to a variable?
var packages = ordered...
for (var package in packages) ...

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/utils.dart:210: var previous = new Map();
{}

And document:

// A map from each node to the node that came before it on the shortest path
from it back to start.

[nweiz, 2013-09-10 21:44:52]

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:67: AssetId
libraryIdentifierToId(String identifier) {
On 2013/09/10 20:31:09, Bob Nystrom wrote:
> Throw an ArgumentError on an empty string.

Throwing a FormatError instead, since it's likely to come from user input.

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:77: final _libraryPathRegexp = new
RegExp(r"^lib/(.*)\.dart$");
On 2013/09/10 20:31:09, Bob Nystrom wrote:
> Capitalize "e" in "exp". (I wish the class was named Regex, but we should be
> consistent with it.)

Done.

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:125: //
multiple cycles we choose which to print deterministically.
On 2013/09/10 20:31:09, Bob Nystrom wrote:
> "deterministically" -> "consistently".

Done.

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:127: in
ordered(graph.packages.values.map((package) => package.name))) {
On 2013/09/10 20:31:09, Bob Nystrom wrote:
> Instead of wrapping this here, how about hoisting the iterator to a variable?
> var packages = ordered...
> for (var package in packages) ...

Done.

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/23924006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/utils.dart:210: var previous = new Map();
On 2013/09/10 20:31:09, Bob Nystrom wrote:
> {}

I thought that implicitly had string keys, but I guess that's not true anymore.
Done.

> And document:
> 
> // A map from each node to the node that came before it on the shortest path
> from it back to start.

Done.

[nweiz, 2013-09-10 21:47:55]

Committed patchset #2 manually as r27355 (presubmit successful).