Allow transformers to exclude and include assets.

Allow transformers to exclude and include assets.

BUG=https://code.google.com/p/dart/issues/detail?id=14594
R=nweiz@google.com

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

[nweiz, 2014-02-24 21:42:45]

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

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:67: /// Each inclusion is an asset
path. If this set is non-empty, than *only*
"than" -> "then"

It seems weird to me that this set being empty is special-cased. I'd rather have
`null` mean "no includes" and the empty set mean "include nothing" (which I can
imagine as a quick-and-dirty way of disabling a transformer, for example).

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:72: /// and excluded, then the set of
included assets is determined, and then
"excluded" -> "excludes"

", and then" -> "and"

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:78: /// Any asset whose patch is in
this is not allowed as a primary input by
"patch" -> "path"

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:82: /// and excluded, then the set of
included assets is determined, and then
Same changes as above

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:84: final Set<String> excludes;
I understand why we're not supporting globs yet, but it really feels like we
should support directories. I think a major use case will be saying things like
"$include: web/real-app". Even when we support globs, forcing people to write
"$include: web/real-app/**/*" is going to be less discoverable and more verbose.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:113: parseField(key, set) {
I hate functions that add elements to collection arguments. It's a little more
forgivable as a local function, but I'd still much rather this just return a
set.

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

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/excluding_transformer.dart:19: Set<String>
excludes) {
Nit, but I think we usually just indent continued lines rather than aligning
them with the first argument. I think that style tends to work better for longer
argument lists or functions where the return type and name are really long.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/excluding_transformer.dart:41: //
TODO(rnystrom): Support globs in addition to paths.
Is there a bug for this? If not, file one and refer to it here.

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

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:392: transformer,
id.includes, id.excludes)).toSet();
Rather than passing the includes and excludes around manually here, maybe just
pass the id. That way we won't have to add more parameters when we add more
built-in configuration stuff.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/test/p...
File sdk/lib/_internal/pub/test/pubspec_test.dart (right):

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/test/p...
sdk/lib/_internal/pub/test/pubspec_test.dart:250: 'Unknown reserved field
"\$key"');
This is a much worse error message than before. It's very valuable to include
the path to the key.

The errors below also don't contain the path.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/test/t...
File
sdk/lib/_internal/pub/test/transformer/exclusion/include_asset_list_test.dart
(right):

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/exclusion/include_asset_list_test.dart:13:
integration("excludes assets", () {
"excludes" -> "includes"

[Bob Nystrom, 2014-02-26 01:09:41]

Thanks!

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

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:67: /// Each inclusion is an asset
path. If this set is non-empty, than *only*
On 2014/02/24 21:42:46, nweiz wrote:
> "than" -> "then"
> 
> It seems weird to me that this set being empty is special-cased. I'd rather
have
> `null` mean "no includes" and the empty set mean "include nothing" (which I
can
> imagine as a quick-and-dirty way of disabling a transformer, for example).

Done.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:72: /// and excluded, then the set of
included assets is determined, and then
On 2014/02/24 21:42:46, nweiz wrote:
> "excluded" -> "excludes"
> 
> ", and then" -> "and"

Done.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:78: /// Any asset whose patch is in
this is not allowed as a primary input by
On 2014/02/24 21:42:46, nweiz wrote:
> "patch" -> "path"

Done.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:82: /// and excluded, then the set of
included assets is determined, and then
On 2014/02/24 21:42:46, nweiz wrote:
> Same changes as above

Done.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:84: final Set<String> excludes;
On 2014/02/24 21:42:46, nweiz wrote:
> I understand why we're not supporting globs yet, but it really feels like we
> should support directories.

I think we should too, but I like smaller, incremental patches.

> Even when we support globs, forcing people to write
> "$include: web/real-app/**/*" is going to be less discoverable and more
verbose.

Filed 17093.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:113: parseField(key, set) {
On 2014/02/24 21:42:46, nweiz wrote:
> I hate functions that add elements to collection arguments. It's a little more
> forgivable as a local function, but I'd still much rather this just return a
> set.

Done.

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

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/excluding_transformer.dart:19: Set<String>
excludes) {
On 2014/02/24 21:42:46, nweiz wrote:
> Nit, but I think we usually just indent continued lines rather than aligning
> them with the first argument. I think that style tends to work better for
longer
> argument lists or functions where the return type and name are really long.

We're inconsistent, I think, but done. I'm fine with moving towards +4 style.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/excluding_transformer.dart:41: //
TODO(rnystrom): Support globs in addition to paths.
On 2014/02/24 21:42:46, nweiz wrote:
> Is there a bug for this? If not, file one and refer to it here.

Done.

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

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:392: transformer,
id.includes, id.excludes)).toSet();
On 2014/02/24 21:42:46, nweiz wrote:
> Rather than passing the includes and excludes around manually here, maybe just
> pass the id. That way we won't have to add more parameters when we add more
> built-in configuration stuff.

Done, except in ExcludingTransformer. It felt weird to pass the id to that.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/test/p...
File sdk/lib/_internal/pub/test/pubspec_test.dart (right):

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/test/p...
sdk/lib/_internal/pub/test/pubspec_test.dart:250: 'Unknown reserved field
"\$key"');
On 2014/02/24 21:42:46, nweiz wrote:
> This is a much worse error message than before. It's very valuable to include
> the path to the key.
> 
> The errors below also don't contain the path.

Done. The paths were always in the error message, the tests just didn't validate
it. Now they do.

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/test/t...
File
sdk/lib/_internal/pub/test/transformer/exclusion/include_asset_list_test.dart
(right):

https://codereview.chromium.org/169223010/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/exclusion/include_asset_list_test.dart:13:
integration("excludes assets", () {
On 2014/02/24 21:42:46, nweiz wrote:
> "excludes" -> "includes"

Done.

[nweiz, 2014-02-26 20:25:07]

lgtm

https://codereview.chromium.org/169223010/diff/30001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/169223010/diff/30001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback.dart:60: /// This will be an empty map if
no configuration was provided.
Is this change left over from when we were considering passing through $-keys?

[Bob Nystrom, 2014-02-26 21:14:22]

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

[Bob Nystrom, 2014-02-26 21:17:02]

https://codereview.chromium.org/169223010/diff/30001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/169223010/diff/30001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback.dart:60: /// This will be an empty map if
no configuration was provided.
On 2014/02/26 20:25:08, nweiz wrote:
> Is this change left over from when we were considering passing through $-keys?

No, this has always been here. I think it's so that transformers don't have to
check for a null config?

[nweiz, 2014-02-26 21:44:03]

https://codereview.chromium.org/169223010/diff/30001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/169223010/diff/30001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback.dart:60: /// This will be an empty map if
no configuration was provided.
On 2014/02/26 21:17:02, Bob Nystrom wrote:
> On 2014/02/26 20:25:08, nweiz wrote:
> > Is this change left over from when we were considering passing through
$-keys?
> 
> No, this has always been here. I think it's so that transformers don't have to
> check for a null config?

I meant the removal of the previous sentence.