Pass in "mode" to transformer plugins.

Pass in "mode" to transformer plugins.

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

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

[Siggi Cherem (dart-lang), 2013-10-30 21:50:45]

lgtm

polymer changes lgtm.

Minor comment/suggestion. The name 'settings.transformer' felt a bit strange to
me. It makes sense, but it's strange because the subject is not an actual
transformer. Other ideas:
 - arguments
 - transformerOptions
 - forTransformer
 - fromTransformer

[Bob Nystrom, 2013-10-30 22:32:34]

On 2013/10/30 21:50:45, Siggi Cherem (dart-lang) wrote:
> lgtm
> 
> polymer changes lgtm.
> 
> Minor comment/suggestion. The name 'settings.transformer' felt a bit strange
to
> me. It makes sense, but it's strange because the subject is not an actual
> transformer. Other ideas:
>  - arguments
>  - transformerOptions
>  - forTransformer
>  - fromTransformer

I had it "transformerConfiguration" at first, but it seemed redundant:
settings.transformerConfiguration. I went for brevity.

- bob

[nweiz, 2013-10-31 00:24:41]

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
File pkg/barback/lib/src/barback_settings.dart (right):

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
pkg/barback/lib/src/barback_settings.dart:17: /// Aside from requiring to be
serializable across isolates, Barback places
"requiring to be" -> "requiring that this be" or "requiring this to be"

Also, technically it's pub that requires this to be serializable. I don't know
if that's a distinction worth making here, though.

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
pkg/barback/lib/src/barback_settings.dart:19: final Map transformer;
I agree with Siggi that this reads awkwardly. What about [forTransformer]?

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
pkg/barback/lib/src/barback_settings.dart:21: /// The overall development mode
that user is running Barback in. This will
Paragraph break.

"development mode" is confusing since "development" is often used as the name of
such a mode. Maybe just remove "development"?

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
pkg/barback/lib/src/barback_settings.dart:48: factory
BarbackMode.fromString(String name) {
It's confusing for this to be a different constructor than [new BarbackMode].
Isn't the other one constructing the mode from a string as well? I'd make this
the default constructor and rename what is now [new BarbackMode] to [new
BarbackMode._].

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

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:111: BarbackMode mode,
Formatting nit: normally when formatting parameters we put them together on
lines as though the "{}" and "[]" for named/optional parameters weren't there,
or else we put each parameter on its own line (again as though the brackets
weren't there). This parameter list should follow one of those two patterns.

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:61: // TODO(rnystrom): Allow
specifying mode.
We should make sure to do this before 1.0 so we don't have to support both
"--mode" and "--minify" forever.

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/test/tr...
File
sdk/lib/_internal/pub/test/transformer/configuration/with_configuration_only_instantiates_configurable_transformers_test.dart
(left):

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/test/tr...
sdk/lib/_internal/pub/test/transformer/configuration/with_configuration_only_instantiates_configurable_transformers_test.dart:1:
// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS d.file
I don't like removing this behavior. If the transformer author has written a
transformer that doesn't take any parameters and the user passes configuration,
that's almost certainly a mistake on someone's part. Continuing as though it's
not will cause bugs or behavioral failures that will be difficult to track down.

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/test/tr...
File
sdk/lib/_internal/pub/test/transformer/fails_to_load_an_unconfigurable_transformer_when_config_is_passed_test.dart
(left):

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/test/tr...
sdk/lib/_internal/pub/test/transformer/fails_to_load_an_unconfigurable_transformer_when_config_is_passed_test.dart:1:
// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS d.file
Also don't like removing this behavior.

[Bob Nystrom, 2013-10-31 01:51:18]

Thanks!

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
File pkg/barback/lib/src/barback_settings.dart (right):

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
pkg/barback/lib/src/barback_settings.dart:17: /// Aside from requiring to be
serializable across isolates, Barback places
On 2013/10/31 00:24:41, nweiz wrote:
> "requiring to be" -> "requiring that this be" or "requiring this to be"
> 
> Also, technically it's pub that requires this to be serializable. I don't know
> if that's a distinction worth making here, though.

Reworded.

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
pkg/barback/lib/src/barback_settings.dart:19: final Map transformer;
On 2013/10/31 00:24:41, nweiz wrote:
> I agree with Siggi that this reads awkwardly. What about [forTransformer]?

I think that's worse. I can do "transformerConfiguration", but that's a
mouthful.

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
pkg/barback/lib/src/barback_settings.dart:21: /// The overall development mode
that user is running Barback in. This will
On 2013/10/31 00:24:41, nweiz wrote:
> Paragraph break.
> 
> "development mode" is confusing since "development" is often used as the name
of
> such a mode. Maybe just remove "development"?

Removed "overall" too.

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
pkg/barback/lib/src/barback_settings.dart:48: factory
BarbackMode.fromString(String name) {
On 2013/10/31 00:24:41, nweiz wrote:
> It's confusing for this to be a different constructor than [new BarbackMode].
> Isn't the other one constructing the mode from a string as well? I'd make this
> the default constructor and rename what is now [new BarbackMode] to [new
> BarbackMode._].

Done. Not sure what I was thinking here.

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

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:111: BarbackMode mode,
On 2013/10/31 00:24:41, nweiz wrote:
> Formatting nit: normally when formatting parameters we put them together on
> lines as though the "{}" and "[]" for named/optional parameters weren't there,
> or else we put each parameter on its own line (again as though the brackets
> weren't there). This parameter list should follow one of those two patterns.

Done. I think. I'm not really sure what the best style is here.

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:61: // TODO(rnystrom): Allow
specifying mode.
On 2013/10/31 00:24:41, nweiz wrote:
> We should make sure to do this before 1.0 so we don't have to support both
> "--mode" and "--minify" forever.

I'm not crazy about coupling minification to mode. I think users may want to
disable minification in pub build, or enable it in pub serve. I'm OK with that
flag sticking around for a good while.

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/test/tr...
File
sdk/lib/_internal/pub/test/transformer/configuration/with_configuration_only_instantiates_configurable_transformers_test.dart
(left):

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/test/tr...
sdk/lib/_internal/pub/test/transformer/configuration/with_configuration_only_instantiates_configurable_transformers_test.dart:1:
// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS d.file
On 2013/10/31 00:24:41, nweiz wrote:
> I don't like removing this behavior. If the transformer author has written a
> transformer that doesn't take any parameters and the user passes
configuration,
> that's almost certainly a mistake on someone's part. Continuing as though it's
> not will cause bugs or behavioral failures that will be difficult to track
down.

Done.

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/test/tr...
File
sdk/lib/_internal/pub/test/transformer/fails_to_load_an_unconfigurable_transformer_when_config_is_passed_test.dart
(left):

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/test/tr...
sdk/lib/_internal/pub/test/transformer/fails_to_load_an_unconfigurable_transformer_when_config_is_passed_test.dart:1:
// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS d.file
On 2013/10/31 00:24:41, nweiz wrote:
> Also don't like removing this behavior.

Done.

[nweiz, 2013-10-31 18:47:56]

I still think "settings.transformer" and "--minify" are both gross, but
otherwise lgtm.

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
File pkg/barback/lib/src/barback_settings.dart (right):

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
pkg/barback/lib/src/barback_settings.dart:19: final Map transformer;
On 2013/10/31 01:51:19, Bob Nystrom wrote:
> On 2013/10/31 00:24:41, nweiz wrote:
> > I agree with Siggi that this reads awkwardly. What about [forTransformer]?
> 
> I think that's worse. I can do "transformerConfiguration", but that's a
> mouthful.

"fromUser"? "fromPubspec"? "pubspec"?

"transformer" really sounds like it's going to return the transformer, or some
information about the transformer. "transformerConfiguration" would be better,
but it's really awfully verbose.

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:61: // TODO(rnystrom): Allow
specifying mode.
On 2013/10/31 01:51:19, Bob Nystrom wrote:
> On 2013/10/31 00:24:41, nweiz wrote:
> > We should make sure to do this before 1.0 so we don't have to support both
> > "--mode" and "--minify" forever.
> 
> I'm not crazy about coupling minification to mode. I think users may want to
> disable minification in pub build, or enable it in pub serve. I'm OK with that
> flag sticking around for a good while.

The question isn't whether you can disable minification in pub build -- it's
whether you can disable minification *and* enable release mode at the same time.
We don't have any evidence that users will want to do that. Even if they do
eventually, I don't think having a "--minify" flag is the right way to let users
configure dart2js separately from other transformers. We don't want to end up
adding pub flags for every option that could conceivably be passed to dart2js;
users should (eventually) configure it the same way they configure other
transformers.

Basically, the "--minify" flag provides no one any value now and won't be the
best way to support its use case in the future.

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

https://codereview.chromium.org/52853004/diff/100001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:76: // If the
constructor expects configuration and none was passed, it defaults    74     var
parameters = [];
What's going on with this line?

[Bob Nystrom, 2013-10-31 20:04:07]

Thanks!

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
File pkg/barback/lib/src/barback_settings.dart (right):

https://codereview.chromium.org/52853004/diff/1/pkg/barback/lib/src/barback_s...
pkg/barback/lib/src/barback_settings.dart:19: final Map transformer;
On 2013/10/31 18:47:56, nweiz wrote:
> On 2013/10/31 01:51:19, Bob Nystrom wrote:
> > On 2013/10/31 00:24:41, nweiz wrote:
> > > I agree with Siggi that this reads awkwardly. What about [forTransformer]?
> > 
> > I think that's worse. I can do "transformerConfiguration", but that's a
> > mouthful.
> 
> "fromUser"? "fromPubspec"? "pubspec"?
> 
> "transformer" really sounds like it's going to return the transformer, or some
> information about the transformer.

Changed to "configuration".

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/52853004/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:61: // TODO(rnystrom): Allow
specifying mode.
On 2013/10/31 18:47:56, nweiz wrote:
> On 2013/10/31 01:51:19, Bob Nystrom wrote:
> > On 2013/10/31 00:24:41, nweiz wrote:
> > > We should make sure to do this before 1.0 so we don't have to support both
> > > "--mode" and "--minify" forever.
> > 
> > I'm not crazy about coupling minification to mode. I think users may want to
> > disable minification in pub build, or enable it in pub serve. I'm OK with
that
> > flag sticking around for a good while.
> 
> The question isn't whether you can disable minification in pub build -- it's
> whether you can disable minification *and* enable release mode at the same
time.
> We don't have any evidence that users will want to do that. Even if they do
> eventually, I don't think having a "--minify" flag is the right way to let
users
> configure dart2js separately from other transformers. We don't want to end up
> adding pub flags for every option that could conceivably be passed to dart2js;
> users should (eventually) configure it the same way they configure other
> transformers.
> 
> Basically, the "--minify" flag provides no one any value now and won't be the
> best way to support its use case in the future.

OK, I'm fine with removing it, but I think that's out of scope for this patch.

From my discussion with Dan this morning, we don't plan to consider command-line
argument changes in tools breaking changes, so we can do this post 1.0 without
much difficulty.

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

https://codereview.chromium.org/52853004/diff/100001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:76: // If the
constructor expects configuration and none was passed, it defaults    74     var
parameters = [];
On 2013/10/31 18:47:57, nweiz wrote:
> What's going on with this line?

Oops, copy/paste error, when I resurrected the previous code here.

[Bob Nystrom, 2013-10-31 20:04:47]

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