Make Transform a little more pleasant to use:

Make Transform a little more pleasant to use:

- Provide the primaryInput asset synchronously (#12515).
- Add convenience methods for reading input assets (#12516).

R=nweiz@google.com, sigmund@google.com

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

[Siggi Cherem (dart-lang), 2013-08-28 00:12:30]

lgtm

thank you =)

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform.dart:71: if (encoding == null) encoding = UTF8;
why not use "encoding: UTF8" as default value above?

https://codereview.chromium.org/23543005/diff/1/pkg/polymer/lib/src/transform...
File pkg/polymer/lib/src/transform/common.dart (left):

https://codereview.chromium.org/23543005/diff/1/pkg/polymer/lib/src/transform...
pkg/polymer/lib/src/transform/common.dart:17: // dartbug.com/12516)
yay!

[Bob Nystrom, 2013-08-28 19:11:33]

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform.dart:71: if (encoding == null) encoding = UTF8;
On 2013/08/28 00:12:30, Siggi Cherem (dart-lang) wrote:
> why not use "encoding: UTF8" as default value above?

If you do that, passing an explicit null will override the default value. That
breaks forwarding, so I always put the default value in the body.

https://codereview.chromium.org/23543005/diff/1/pkg/polymer/lib/src/transform...
File pkg/polymer/lib/src/transform/common.dart (left):

https://codereview.chromium.org/23543005/diff/1/pkg/polymer/lib/src/transform...
pkg/polymer/lib/src/transform/common.dart:17: // dartbug.com/12516)
On 2013/08/28 00:12:30, Siggi Cherem (dart-lang) wrote:
> yay!

\o/

[nweiz, 2013-08-28 19:29:43]

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform.dart:51: assert(_node.primary.state ==
AssetState.AVAILABLE);
This shouldn't be an assertion. It's expected to fail occasionally if the
transformer accesses primaryInput asynchronously, so it should throw a real
exception (probably StateError).

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform.dart:65: /// This is equivalent to
`getInput().readAsString()`.
`getInput().readAsString()` is a little confusing, since [getInput] returns a
Future. Same below.

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform.dart:81: getInput(id).then((input) =>
input.read());
[Asset.read] returns a Stream<List<int>>, not a Future<List<int>>. You could use
the [futureStream] function in pkg/scheduled_test/lib/src/utils.dart to just
have this return a Stream<List<int>>, though.

https://codereview.chromium.org/23543005/diff/1/pkg/barback/test/transformer/...
File pkg/barback/test/transformer/mock.dart (left):

https://codereview.chromium.org/23543005/diff/1/pkg/barback/test/transformer/...
pkg/barback/test/transformer/mock.dart:135: Future<Asset> getPrimary(Transform
transform) =>
We should keep this around so [pauseGetInput] still works for primary inputs.

[Bob Nystrom, 2013-08-28 20:51:27]

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform.dart:51: assert(_node.primary.state ==
AssetState.AVAILABLE);
On 2013/08/28 19:29:43, nweiz wrote:
> This shouldn't be an assertion. It's expected to fail occasionally if the
> transformer accesses primaryInput asynchronously, so it should throw a real
> exception (probably StateError).

Done.

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform.dart:65: /// This is equivalent to
`getInput().readAsString()`.
On 2013/08/28 19:29:43, nweiz wrote:
> `getInput().readAsString()` is a little confusing, since [getInput] returns a
> Future. Same below.

Reworded a bit. I didn't want to do the full async code for it since it's
pointlessly verbose.

https://codereview.chromium.org/23543005/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform.dart:81: getInput(id).then((input) =>
input.read());
On 2013/08/28 19:29:43, nweiz wrote:
> [Asset.read] returns a Stream<List<int>>, not a Future<List<int>>. You could
use
> the [futureStream] function in pkg/scheduled_test/lib/src/utils.dart to just
> have this return a Stream<List<int>>, though.

Done.

https://codereview.chromium.org/23543005/diff/1/pkg/barback/test/transformer/...
File pkg/barback/test/transformer/mock.dart (left):

https://codereview.chromium.org/23543005/diff/1/pkg/barback/test/transformer/...
pkg/barback/test/transformer/mock.dart:135: Future<Asset> getPrimary(Transform
transform) =>
On 2013/08/28 19:29:43, nweiz wrote:
> We should keep this around so [pauseGetInput] still works for primary inputs.

Done, but modified to actually call primaryInput instead of just getInput() to
cover that code path.

[nweiz, 2013-08-28 21:15:00]

A couple suggestions, otherwise lgtm.

https://codereview.chromium.org/23543005/diff/8001/pkg/barback/lib/src/transf...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/23543005/diff/8001/pkg/barback/lib/src/transf...
pkg/barback/lib/src/transform.dart:55: /// of running, it may be unavailable
when this transformer later resumes.
This sentence is confusing and slightly inaccurate. It's not just the previous
transform that can cause the input to change; it could be a source input. I
don't think it's worth trying to explain the full reasoning that the input may
not be available here. It's probably sufficient just to say that it may be
unavailable.

https://codereview.chromium.org/23543005/diff/8001/pkg/barback/test/transform...
File pkg/barback/test/transformer/mock.dart (right):

https://codereview.chromium.org/23543005/diff/8001/pkg/barback/test/transform...
pkg/barback/test/transformer/mock.dart:130: void pauseGetPrimary() {
I don't like making these separate from [pauseGetInput] and [resumeGetInput].
They're visually difficult to distinguish from {pause,resume}IsPrimary, and they
mean that calling `pauseGetInput(primaryId)` won't work as expected.

[Bob Nystrom, 2013-08-28 22:37:48]

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

[Bob Nystrom, 2013-08-28 22:38:24]

https://codereview.chromium.org/23543005/diff/8001/pkg/barback/lib/src/transf...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/23543005/diff/8001/pkg/barback/lib/src/transf...
pkg/barback/lib/src/transform.dart:55: /// of running, it may be unavailable
when this transformer later resumes.
On 2013/08/28 21:15:00, nweiz wrote:
> This sentence is confusing and slightly inaccurate. It's not just the previous
> transform that can cause the input to change; it could be a source input. I
> don't think it's worth trying to explain the full reasoning that the input may
> not be available here. It's probably sufficient just to say that it may be
> unavailable.

Re-worded a bit.

https://codereview.chromium.org/23543005/diff/8001/pkg/barback/test/transform...
File pkg/barback/test/transformer/mock.dart (right):

https://codereview.chromium.org/23543005/diff/8001/pkg/barback/test/transform...
pkg/barback/test/transformer/mock.dart:130: void pauseGetPrimary() {
On 2013/08/28 21:15:00, nweiz wrote:
> I don't like making these separate from [pauseGetInput] and [resumeGetInput].
> They're visually difficult to distinguish from {pause,resume}IsPrimary, and
they
> mean that calling `pauseGetInput(primaryId)` won't work as expected.

I think it's important to split this out. This method hits the primaryInput
getter where getInput() does not. I want coverage on that codepath. I could make
pauseInput() check the ID and call primaryInput if the ID is the primary one,
but I think that obscures the intent of the tests.

I renamed the methods to pausePrimaryInput() and resumePrimaryInput() to be a
bit more distinct.