Don't require Transformer methods to return Futures.

Don't require Transformer methods to return Futures.

R=rnystrom@google.com

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

[Bob Nystrom, 2014-04-21 22:32:23]

https://codereview.chromium.org/243793005/diff/1/pkg/barback/lib/src/declarin...
File pkg/barback/lib/src/declaring_transformer.dart (right):

https://codereview.chromium.org/243793005/diff/1/pkg/barback/lib/src/declarin...
pkg/barback/lib/src/declaring_transformer.dart:26: /// until that [Future]
completes.
I think we need to word this more strongly. Users need to understand that if the
body of this does asynchronous work, it *should* return a future.

https://codereview.chromium.org/243793005/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer.dart (right):

https://codereview.chromium.org/243793005/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer.dart:63: /// This may return either a `bool` or
a `Future<bool>`.
Here too. Make it clear that not returning a future is only valid if the
implementation does nothing async.

https://codereview.chromium.org/243793005/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer.dart:88: /// until that [Future] completes.
Ditto.

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/bad.dart (right):

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/bad.dart:13: class BadTransformer extends
MockTransformer {
A lot (all?) of the test transformers are sync now. Do we still have test
coverage for async transformers?

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer_test.dart (right):

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer_test.dart:20: var transformer = new
ExtensionTransformer(".txt .bin");
Why not just make these sync?

[nweiz, 2014-04-21 22:46:24]

https://codereview.chromium.org/243793005/diff/1/pkg/barback/lib/src/declarin...
File pkg/barback/lib/src/declaring_transformer.dart (right):

https://codereview.chromium.org/243793005/diff/1/pkg/barback/lib/src/declarin...
pkg/barback/lib/src/declaring_transformer.dart:26: /// until that [Future]
completes.
On 2014/04/21 22:32:23, Bob Nystrom wrote:
> I think we need to word this more strongly. Users need to understand that if
the
> body of this does asynchronous work, it *should* return a future.

Done.

https://codereview.chromium.org/243793005/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer.dart (right):

https://codereview.chromium.org/243793005/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer.dart:63: /// This may return either a `bool` or
a `Future<bool>`.
On 2014/04/21 22:32:23, Bob Nystrom wrote:
> Here too. Make it clear that not returning a future is only valid if the
> implementation does nothing async.

Done.

https://codereview.chromium.org/243793005/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer.dart:88: /// until that [Future] completes.
On 2014/04/21 22:32:23, Bob Nystrom wrote:
> Ditto.

Done.

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/bad.dart (right):

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/bad.dart:13: class BadTransformer extends
MockTransformer {
On 2014/04/21 22:32:23, Bob Nystrom wrote:
> A lot (all?) of the test transformers are sync now. Do we still have test
> coverage for async transformers?

They're actually still async, since they're wrapped by MockTransformer.

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer_test.dart (right):

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer_test.dart:20: var transformer = new
ExtensionTransformer(".txt .bin");
On 2014/04/21 22:32:23, Bob Nystrom wrote:
> Why not just make these sync?

Done.

[Bob Nystrom, 2014-04-21 23:37:59]

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/bad.dart (right):

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/bad.dart:13: class BadTransformer extends
MockTransformer {
On 2014/04/21 22:46:24, nweiz wrote:
> On 2014/04/21 22:32:23, Bob Nystrom wrote:
> > A lot (all?) of the test transformers are sync now. Do we still have test
> > coverage for async transformers?
> 
> They're actually still async, since they're wrapped by MockTransformer.

Oh, well, in that case do we have any sync transformer test coverage? :)

[nweiz, 2014-04-21 23:49:41]

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/bad.dart (right):

https://codereview.chromium.org/243793005/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/bad.dart:13: class BadTransformer extends
MockTransformer {
On 2014/04/21 23:38:00, Bob Nystrom wrote:
> On 2014/04/21 22:46:24, nweiz wrote:
> > On 2014/04/21 22:32:23, Bob Nystrom wrote:
> > > A lot (all?) of the test transformers are sync now. Do we still have test
> > > coverage for async transformers?
> > 
> > They're actually still async, since they're wrapped by MockTransformer.
> 
> Oh, well, in that case do we have any sync transformer test coverage? :)

I guess not. Done.

[Bob Nystrom, 2014-04-22 18:23:49]

LGTM.

https://codereview.chromium.org/243793005/diff/30001/pkg/barback/test/package...
File pkg/barback/test/package_graph/transform/transform_test.dart (right):

https://codereview.chromium.org/243793005/diff/30001/pkg/barback/test/package...
pkg/barback/test/package_graph/transform/transform_test.dart:345: test("gets a
asset transformed by a sync transformer", () {
"a asset" -> "an asset".

[nweiz, 2014-04-22 19:57:56]

https://codereview.chromium.org/243793005/diff/30001/pkg/barback/test/package...
File pkg/barback/test/package_graph/transform/transform_test.dart (right):

https://codereview.chromium.org/243793005/diff/30001/pkg/barback/test/package...
pkg/barback/test/package_graph/transform/transform_test.dart:345: test("gets a
asset transformed by a sync transformer", () {
On 2014/04/22 18:23:49, Bob Nystrom wrote:
> "a asset" -> "an asset".

Done.

[nweiz, 2014-04-22 19:59:45]

Committed patchset #4 manually as r35280 (presubmit successful).