pkg/test: Resolve a number of analyzer warnings

Resolve a number of analyzer warnings

Hide strong-mode warnings in test directory
Remove unused imports and fields
Consolidate line splitter instances

R=nweiz@google.com

Committed: https://github.com/dart-lang/test/commit/98b9c745640aae0eb21200c3ca1a74dd96fd3b11

[kevmoo, 2016-06-15 21:04:24]

kevmoo@google.com changed reviewers:
+ nweiz@google.com

[nweiz, 2016-06-20 20:51:36]

I assume this is changes for 1.17? Does the code still pass 1.16 analysis?

https://codereview.chromium.org/2068363002/diff/1/.analysis_options
File .analysis_options (right):

https://codereview.chromium.org/2068363002/diff/1/.analysis_options#newcode4
.analysis_options:4: - test/**/*.dart
What does this do exactly? Is there a way to run the analyzer on all files in a
package now?

https://codereview.chromium.org/2068363002/diff/1/lib/src/runner/browser/cont...
File lib/src/runner/browser/content_shell.dart (right):

https://codereview.chromium.org/2068363002/diff/1/lib/src/runner/browser/cont...
lib/src/runner/browser/content_shell.dart:108: var json = await
JSON.fuse(UTF8).decoder.bind(response).single as List;
Why doesn't this complain about calling operator [] on the result of json.first?

[kevmoo, 2016-06-20 20:57:49]

This change addresses things from 1.18-dev.1 analysis

Will rerun on 1.17 analysis when I'm on a faster connection

https://codereview.chromium.org/2068363002/diff/1/.analysis_options
File .analysis_options (right):

https://codereview.chromium.org/2068363002/diff/1/.analysis_options#newcode4
.analysis_options:4: - test/**/*.dart
On 2016/06/20 20:51:36, nweiz wrote:
> What does this do exactly? Is there a way to run the analyzer on all files in
a
> package now?

This excludes the files in the test directory from *strong* analysis only.

Weird, I know.

https://codereview.chromium.org/2068363002/diff/1/lib/src/runner/browser/cont...
File lib/src/runner/browser/content_shell.dart (right):

https://codereview.chromium.org/2068363002/diff/1/lib/src/runner/browser/cont...
lib/src/runner/browser/content_shell.dart:108: var json = await
JSON.fuse(UTF8).decoder.bind(response).single as List;
On 2016/06/20 20:51:36, nweiz wrote:
> Why doesn't this complain about calling operator [] on the result of
json.first?

You get Object out of the fused decoder, which is not a List.

Each item in `List` is dynamic, though – so of course it has an index operator

[kevmoo, 2016-06-22 18:26:48]

On 2016/06/20 20:57:49, kevmoo wrote:
> This change addresses things from 1.18-dev.1 analysis
> 
> Will rerun on 1.17 analysis when I'm on a faster connection
> 
> https://codereview.chromium.org/2068363002/diff/1/.analysis_options
> File .analysis_options (right):
> 
> https://codereview.chromium.org/2068363002/diff/1/.analysis_options#newcode4
> .analysis_options:4: - test/**/*.dart
> On 2016/06/20 20:51:36, nweiz wrote:
> > What does this do exactly? Is there a way to run the analyzer on all files
in
> a
> > package now?
> 
> This excludes the files in the test directory from *strong* analysis only.
> 
> Weird, I know.
> 
>
https://codereview.chromium.org/2068363002/diff/1/lib/src/runner/browser/cont...
> File lib/src/runner/browser/content_shell.dart (right):
> 
>
https://codereview.chromium.org/2068363002/diff/1/lib/src/runner/browser/cont...
> lib/src/runner/browser/content_shell.dart:108: var json = await
> JSON.fuse(UTF8).decoder.bind(response).single as List;
> On 2016/06/20 20:51:36, nweiz wrote:
> > Why doesn't this complain about calling operator [] on the result of
> json.first?
> 
> You get Object out of the fused decoder, which is not a List.
> 
> Each item in `List` is dynamic, though – so of course it has an index
operator

ping?

[kevmoo, 2016-06-23 21:37:26]

PTAL - changed the helper to just wrap the necessary calls to bind

[nweiz, 2016-06-23 21:47:37]

https://codereview.chromium.org/2068363002/diff/20001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/2068363002/diff/20001/lib/src/utils.dart#newc...
lib/src/utils.dart:32: source.transform(UTF8.decoder).transform(const
LineSplitter());
You can make this an actual StreamTransformer using new StreamTransformer:
https://api.dartlang.org/stable/1.17.1/dart-async/StreamTransformer/StreamTra...

[kevmoo, 2016-06-23 21:57:08]

https://codereview.chromium.org/2068363002/diff/20001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/2068363002/diff/20001/lib/src/utils.dart#newc...
lib/src/utils.dart:32: source.transform(UTF8.decoder).transform(const
LineSplitter());
On 2016/06/23 21:47:36, nweiz wrote:
> You can make this an actual StreamTransformer using new StreamTransformer:
>
https://api.dartlang.org/stable/1.17.1/dart-async/StreamTransformer/StreamTra...

Yes, but is there an upside to doing so?

Also, I'm struggling to figure out an easy way to do it.

final StreamTransformer<List<int>, String> transformer = new StreamTransformer(
    (Stream<List<int>> stream, cancelOnError) => stream
            .transform(UTF8.decoder)
            .transform(const LineSplitter())
            .listen((s) {
          // now what?
        }));

[nweiz, 2016-06-23 22:02:57]

https://codereview.chromium.org/2068363002/diff/20001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/2068363002/diff/20001/lib/src/utils.dart#newc...
lib/src/utils.dart:32: source.transform(UTF8.decoder).transform(const
LineSplitter());
On 2016/06/23 21:57:08, kevmoo wrote:
> On 2016/06/23 21:47:36, nweiz wrote:
> > You can make this an actual StreamTransformer using new StreamTransformer:
> >
>
https://api.dartlang.org/stable/1.17.1/dart-async/StreamTransformer/StreamTra...
> 
> Yes, but is there an upside to doing so?

The API for using it is nicer and produces less churn.

> Also, I'm struggling to figure out an easy way to do it.
> 
> final StreamTransformer<List<int>, String> transformer = new
StreamTransformer(
>     (Stream<List<int>> stream, cancelOnError) => stream
>             .transform(UTF8.decoder)
>             .transform(const LineSplitter())
>             .listen((s) {
>           // now what?
>         }));

You pass null to the listen callback and return its result.

[kevmoo, 2016-06-23 22:12:24]

https://codereview.chromium.org/2068363002/diff/20001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/2068363002/diff/20001/lib/src/utils.dart#newc...
lib/src/utils.dart:32: source.transform(UTF8.decoder).transform(const
LineSplitter());
On 2016/06/23 22:02:56, nweiz wrote:
> On 2016/06/23 21:57:08, kevmoo wrote:
> > On 2016/06/23 21:47:36, nweiz wrote:
> > > You can make this an actual StreamTransformer using new StreamTransformer:
> > >
> >
>
https://api.dartlang.org/stable/1.17.1/dart-async/StreamTransformer/StreamTra...
> > 
> > Yes, but is there an upside to doing so?
> 
> The API for using it is nicer and produces less churn.
> 
> > Also, I'm struggling to figure out an easy way to do it.
> > 
> > final StreamTransformer<List<int>, String> transformer = new
> StreamTransformer(
> >     (Stream<List<int>> stream, cancelOnError) => stream
> >             .transform(UTF8.decoder)
> >             .transform(const LineSplitter())
> >             .listen((s) {
> >           // now what?
> >         }));
> 
> You pass null to the listen callback and return its result.

Done.

[nweiz, 2016-06-23 22:22:48]

Is this strong-mode clean on 1.16 and 1.17?

https://codereview.chromium.org/2068363002/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/2068363002/diff/40001/lib/src/utils.dart#newc...
lib/src/utils.dart:31: final StreamTransformer<List<int>, String> lineSplitter =
new StreamTransformer(
You can probably avoid type-annotating this, too, if you call
UTF8.decoder.bind() in the callback rather than stream.transform().

https://codereview.chromium.org/2068363002/diff/40001/lib/src/utils.dart#newc...
lib/src/utils.dart:32: (Stream<List<int>> stream, bool cancelOnError) => stream
Don't type-annotate these. They should be inferred.

[kevmoo, 2016-06-23 22:32:29]

https://codereview.chromium.org/2068363002/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/2068363002/diff/40001/lib/src/utils.dart#newc...
lib/src/utils.dart:31: final StreamTransformer<List<int>, String> lineSplitter =
new StreamTransformer(
On 2016/06/23 22:22:47, nweiz wrote:
> You can probably avoid type-annotating this, too, if you call
> UTF8.decoder.bind() in the callback rather than stream.transform().

Just putting types on the right seems to do the trick

https://codereview.chromium.org/2068363002/diff/40001/lib/src/utils.dart#newc...
lib/src/utils.dart:32: (Stream<List<int>> stream, bool cancelOnError) => stream
On 2016/06/23 22:22:47, nweiz wrote:
> Don't type-annotate these. They should be inferred.

Done.

[nweiz, 2016-06-23 22:33:58]

LGTM!

[kevmoo, 2016-06-23 22:35:23]

Description was changed from

==========
Resolve a number of analyzer warnings

Hide strong-mode warnings in test directory
Remove unused imports and fields
Consolidate line splitter instances
==========

to

==========
Resolve a number of analyzer warnings

Hide strong-mode warnings in test directory
Remove unused imports and fields
Consolidate line splitter instances

R=nweiz@google.com

Committed:
https://github.com/dart-lang/test/commit/98b9c745640aae0eb21200c3ca1a74dd96fd...
==========

[kevmoo, 2016-06-23 22:35:24]

Committed patchset #4 (id:60001) manually as
98b9c745640aae0eb21200c3ca1a74dd96fd3b11 (presubmit successful).