Expose aggregate transformers in pub.

Expose aggregate transformers in pub.

This adds barback-0.14.0+3 to the suite of legacy barbacks that pub is tested
against.

BUG=18948
R=rnystrom@google.com

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

[ricow1, 2014-06-04 09:31:16]

https://codereview.chromium.org/310233002/diff/1/deps/all.deps/DEPS
File deps/all.deps/DEPS (right):

https://codereview.chromium.org/310233002/diff/1/deps/all.deps/DEPS#newcode108
deps/all.deps/DEPS:108: # Thes specific versions of barback are used for testing
and should be pulled
Thes -> These

[nweiz, 2014-06-04 20:19:52]

https://codereview.chromium.org/310233002/diff/1/deps/all.deps/DEPS
File deps/all.deps/DEPS (right):

https://codereview.chromium.org/310233002/diff/1/deps/all.deps/DEPS#newcode108
deps/all.deps/DEPS:108: # Thes specific versions of barback are used for testing
and should be pulled
On 2014/06/04 09:31:16, ricow1 wrote:
> Thes -> These

Done.

[Bob Nystrom, 2014-06-09 22:05:49]

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File dart/sdk/lib/_internal/pub/asset/dart/serialize/aggregate_transform.dart
(right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/aggregate_transform.dart:79: },
{
What about getInput and addOutput?

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/aggregate_transform.dart:120:
//# if barback >=0.14.1-dev
Is there are reason this needs to be limited, but the above doesn't? It might be
good to leave some documentation here explaining what's missing/adding in
various barback versions to determine this.

Also, do you plan to remove the "-dev" here when you rev pub itself?

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/aggregate_transform.dart:124:
class ForeignAggregateTransform extends _ForeignBaseAggregateTransform
Nit, but I don't think it's worth indenting stuff inside the ifdefs.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/aggregate_transform.dart:141:
Future<String> readInputAsString(AssetId id, {Encoding encoding}) {
Can these be put in a mixin? The non-aggregate class has something identical,
doesn't it?

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File dart/sdk/lib/_internal/pub/asset/dart/serialize/transformer.dart (right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/transformer.dart:107: ///
Converts [transformerLike] into a serializable map.
Document what "-like" is.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/transformer.dart:115: // throw
on
throw on?

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File dart/sdk/lib/_internal/pub/asset/dart/transformer_isolate.dart (right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/transformer_isolate.dart:93: //# if
barback >=0.14.1-dev
Document this.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File
dart/sdk/lib/_internal/pub/lib/src/barback/excluding_aggregate_transformer.dart
(right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/lib/src/barback/excluding_aggregate_transformer.dart:13:
class ExcludingAggregateTransformer extends AggregateTransformer {
Obviously classifyPrimary() can't be, but can most of this be pushed into a
mixin shared with the non-aggregate excluding class?

I feel like we're starting to get into a hairy parallel class hierarchy
situation where there's a lot of copy/pasted code along the
lazy/declaring/eager, aggregate/singular, and excluding/normal axes.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File
dart/sdk/lib/_internal/pub/test/transformer/exclusion/works_on_aggregate_transformer_test.dart
(right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/test/transformer/exclusion/works_on_aggregate_transformer_test.dart:27:
assets.sort((asset1, asset2) => asset1.id.path.compareTo(asset2.id.path));
I wonder if it's worth having a sort() utility method that takes a map fn for
the elements:

sortBy(collection, map(value)) => collection.sort((a, b) =>
map(a).compareTo(map(b));

I feel like this pattern is pretty common, at least in general. I don't know if
it appears in pub enough to warrant adding it.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/test/transformer/exclusion/works_on_aggregate_transformer_test.dart:49:
"\$exclude": "web/a.txt"
Would be good to add one more included asset so that we can see that the
aggregation does happen.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File
dart/sdk/lib/_internal/pub/test/transformer/loads_an_aggregate_transformer_test.dart
(right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/test/transformer/loads_an_aggregate_transformer_test.dart:59:
requestShouldSucceed("out.txt", "bar\nfoo");
Shouldn't this be "\\n" too?

[nweiz, 2014-06-10 19:19:36]

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File dart/sdk/lib/_internal/pub/asset/dart/serialize/aggregate_transform.dart
(right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/aggregate_transform.dart:79: },
{
On 2014/06/09 22:05:48, Bob Nystrom wrote:
> What about getInput and addOutput?

Those aren't supported for declaring transforms.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/aggregate_transform.dart:120:
//# if barback >=0.14.1-dev
On 2014/06/09 22:05:48, Bob Nystrom wrote:
> Is there are reason this needs to be limited, but the above doesn't? It might
be
> good to leave some documentation here explaining what's missing/adding in
> various barback versions to determine this.

Done.

> Also, do you plan to remove the "-dev" here when you rev pub itself?

Yes.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/aggregate_transform.dart:124:
class ForeignAggregateTransform extends _ForeignBaseAggregateTransform
On 2014/06/09 22:05:48, Bob Nystrom wrote:
> Nit, but I don't think it's worth indenting stuff inside the ifdefs.

Done.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/aggregate_transform.dart:141:
Future<String> readInputAsString(AssetId id, {Encoding encoding}) {
On 2014/06/09 22:05:48, Bob Nystrom wrote:
> Can these be put in a mixin? The non-aggregate class has something identical,
> doesn't it?

Done.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File dart/sdk/lib/_internal/pub/asset/dart/serialize/transformer.dart (right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/transformer.dart:107: ///
Converts [transformerLike] into a serializable map.
On 2014/06/09 22:05:48, Bob Nystrom wrote:
> Document what "-like" is.

Done.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/serialize/transformer.dart:115: // throw
on
On 2014/06/09 22:05:48, Bob Nystrom wrote:
> throw on?

Done.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File dart/sdk/lib/_internal/pub/asset/dart/transformer_isolate.dart (right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/asset/dart/transformer_isolate.dart:93: //# if
barback >=0.14.1-dev
On 2014/06/09 22:05:48, Bob Nystrom wrote:
> Document this.

Done.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File
dart/sdk/lib/_internal/pub/lib/src/barback/excluding_aggregate_transformer.dart
(right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/lib/src/barback/excluding_aggregate_transformer.dart:13:
class ExcludingAggregateTransformer extends AggregateTransformer {
On 2014/06/09 22:05:48, Bob Nystrom wrote:
> Obviously classifyPrimary() can't be, but can most of this be pushed into a
> mixin shared with the non-aggregate excluding class?
> 
> I feel like we're starting to get into a hairy parallel class hierarchy
> situation where there's a lot of copy/pasted code along the
> lazy/declaring/eager, aggregate/singular, and excluding/normal axes.

There aren't really any instance methods in common between
ExcludingAggregateTransformer and ExcludingTransformer other than [toString].

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File
dart/sdk/lib/_internal/pub/test/transformer/exclusion/works_on_aggregate_transformer_test.dart
(right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/test/transformer/exclusion/works_on_aggregate_transformer_test.dart:27:
assets.sort((asset1, asset2) => asset1.id.path.compareTo(asset2.id.path));
On 2014/06/09 22:05:48, Bob Nystrom wrote:
> I wonder if it's worth having a sort() utility method that takes a map fn for
> the elements:
> 
> sortBy(collection, map(value)) => collection.sort((a, b) =>
> map(a).compareTo(map(b));
> 
> I feel like this pattern is pretty common, at least in general. I don't know
if
> it appears in pub enough to warrant adding it.

Ruby has it in the standard library! I think this might be nice to add, but I
don't think the big search-and-replace belongs in this CL.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/test/transformer/exclusion/works_on_aggregate_transformer_test.dart:49:
"\$exclude": "web/a.txt"
On 2014/06/09 22:05:48, Bob Nystrom wrote:
> Would be good to add one more included asset so that we can see that the
> aggregation does happen.

Done.

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File
dart/sdk/lib/_internal/pub/test/transformer/loads_an_aggregate_transformer_test.dart
(right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/test/transformer/loads_an_aggregate_transformer_test.dart:59:
requestShouldSucceed("out.txt", "bar\nfoo");
On 2014/06/09 22:05:48, Bob Nystrom wrote:
> Shouldn't this be "\\n" too?

No; the previous one is double-escaped because it's in a multiline string, but
this isn't.

[Bob Nystrom, 2014-06-10 19:44:40]

lgtm

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
File
dart/sdk/lib/_internal/pub/test/transformer/loads_an_aggregate_transformer_test.dart
(right):

https://codereview.chromium.org/310233002/diff/20001/dart/sdk/lib/_internal/p...
dart/sdk/lib/_internal/pub/test/transformer/loads_an_aggregate_transformer_test.dart:59:
requestShouldSucceed("out.txt", "bar\nfoo");
On 2014/06/10 19:19:36, nweiz wrote:
> On 2014/06/09 22:05:48, Bob Nystrom wrote:
> > Shouldn't this be "\\n" too?
> 
> No; the previous one is double-escaped because it's in a multiline string, but
> this isn't.

Oh, right.

[nweiz, 2014-06-10 20:37:41]

Committed patchset #3 manually as r37199 (presubmit successful).