Support passing configuration data to a transformer plugin.

Support passing configuration data to a transformer plugin.

This also requires that transformer plugins define an [asPlugin] named
constructor in order to be loaded as plugins.

R=rnystrom@google.com

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

[Bob Nystrom, 2013-09-11 22:09:20]

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

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:41: '"exclude".');
Refactor this to get rid of the duplication:

for (var reserved in ["include", "exclude"]) { ... }

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

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:300:
'configuration': JSON.encode(id.configuration)
I don't think we need to encode/decode here. JSON objects can already be sent
over isolates in object form.

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

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:194: 'Transformer "$transformer" must
be a string or map.');
Add a test to pubspec_test for this.

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:211: 'be a map, but was
"$configuration".');
And this.

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/with_configuration_only_instantiates_configurable_transformers_test.dart
(right):

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/with_configuration_only_instantiates_configurable_transformers_test.dart:52:
integration("with configuration, only instantiates configurable transformers",
This is probably an edge case that won't occur much in practice, but my
intuition is that this would instantiate both transformers. Since
RewriteTransformer doesn't use the config, it would just be ignored.

[nweiz, 2013-09-12 00:30:34]

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

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:41: '"exclude".');
On 2013/09/11 22:09:20, Bob Nystrom wrote:
> Refactor this to get rid of the duplication:
> 
> for (var reserved in ["include", "exclude"]) { ... }

Done.

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

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:300:
'configuration': JSON.encode(id.configuration)
On 2013/09/11 22:09:20, Bob Nystrom wrote:
> I don't think we need to encode/decode here. JSON objects can already be sent
> over isolates in object form.

The configuration is parsed from Yaml, which returns a custom YamlMap object
that supports non-String keys (see the TODO). If we send that over, the VM
segfaults.

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

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:194: 'Transformer "$transformer" must
be a string or map.');
On 2013/09/11 22:09:20, Bob Nystrom wrote:
> Add a test to pubspec_test for this.

Done.

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:211: 'be a map, but was
"$configuration".');
On 2013/09/11 22:09:20, Bob Nystrom wrote:
> And this.

Done.

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/with_configuration_only_instantiates_configurable_transformers_test.dart
(right):

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/with_configuration_only_instantiates_configurable_transformers_test.dart:52:
integration("with configuration, only instantiates configurable transformers",
On 2013/09/11 22:09:20, Bob Nystrom wrote:
> This is probably an edge case that won't occur much in practice, but my
> intuition is that this would instantiate both transformers. Since
> RewriteTransformer doesn't use the config, it would just be ignored.

If there's only one transformer in a library, that transformer doesn't take any
configuration, and it's instantiated with configuration, that should be an error
-- the pubspec author clearly expects that their configuration will be used for
something, so silently ignoring it is wrong.

Given that, I think it makes sense for transforms that don't take configuration
to consistently never be instantiated when configuration is passed. Given how
edgy of an edge case this is, I think it's better to have a consistent rule for
behavior than to optimize for what seems correct when you look at this case in a
vaccum.

[Bob Nystrom, 2013-09-12 00:32:18]

LGTM!

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/with_configuration_only_instantiates_configurable_transformers_test.dart
(right):

https://codereview.chromium.org/23522029/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/with_configuration_only_instantiates_configurable_transformers_test.dart:52:
integration("with configuration, only instantiates configurable transformers",
On 2013/09/12 00:30:34, nweiz wrote:
> On 2013/09/11 22:09:20, Bob Nystrom wrote:
> > This is probably an edge case that won't occur much in practice, but my
> > intuition is that this would instantiate both transformers. Since
> > RewriteTransformer doesn't use the config, it would just be ignored.
> 
> If there's only one transformer in a library, that transformer doesn't take
any
> configuration, and it's instantiated with configuration, that should be an
error
> -- the pubspec author clearly expects that their configuration will be used
for
> something, so silently ignoring it is wrong.
> 
> Given that, I think it makes sense for transforms that don't take
configuration
> to consistently never be instantiated when configuration is passed. Given how
> edgy of an edge case this is, I think it's better to have a consistent rule
for
> behavior than to optimize for what seems correct when you look at this case in
a
> vaccum.

SGTM.

[nweiz, 2013-09-12 19:41:15]

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