fix observe package to be strong mode clean

fix observe package to be strong mode clean

R=sigmund@google.com

Committed: https://github.com/dart-lang/observe/commit/fa9ac07ceaa44267fc4aacbede7c78c5e908f347

[Jennifer Messerly, 2016-03-25 17:12:58]

jmesserly@google.com changed reviewers:
+ jakemac@google.com, sigmund@google.com

[Siggi Cherem (dart-lang), 2016-03-25 17:36:29]

lgtm

https://codereview.chromium.org/1838473002/diff/20001/lib/transformer.dart
File lib/transformer.dart (right):

https://codereview.chromium.org/1838473002/diff/20001/lib/transformer.dart#ne...
lib/transformer.dart:49: files = new List<String>.from(value);
consider adding a TODO referring to
https://github.com/dart-lang/collection/issues/25? (basically whenever we are
cloning to make the type-system happy, we may want to convert the code to use a
type-coercion wrapper in the future)

https://codereview.chromium.org/1838473002/diff/20001/lib/transformer.dart#ne...
lib/transformer.dart:261: token?.type == TokenType.KEYWORD && token.lexeme ==
keyword.syntax;
was this broken?

https://codereview.chromium.org/1838473002/diff/20001/test/observe_test_utils...
File test/observe_test_utils.dart (right):

https://codereview.chromium.org/1838473002/diff/20001/test/observe_test_utils...
test/observe_test_utils.dart:46: new List.from(changes.where((c) =>
c.indexChanged(index)));
I'm surprised that you need to replace `.toList` with `new List.from` here.
Doesn't `toList()` preserve the type argument?

https://codereview.chromium.org/1838473002/diff/20001/test/transformer_test.dart
File test/transformer_test.dart (right):

https://codereview.chromium.org/1838473002/diff/20001/test/transformer_test.d...
test/transformer_test.dart:129: Future _transform(String code) {
Doesn't matter here, but would we be able to keep String if we change should
Chain.capture to have a generic type like:

    <T> static T capture(T callback(), ... );

?

https://codereview.chromium.org/1838473002/diff/20001/test/transformer_test.d...
test/transformer_test.dart:137: await transformer.apply(transform);
is the change to use await necessary for now?

I mean, is this because we don't have union types and Future.then is
practically:

class Future<T> {
  <E> then({E | Future<E>} callback(T));
}

[jakemac, 2016-03-25 17:44:13]

https://codereview.chromium.org/1838473002/diff/20001/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/1838473002/diff/20001/pubspec.yaml#newcode29
pubspec.yaml:29: sdk: ^1.8.0
Does this sdk support the ^ syntax? I think it might be some 1.9 release

[Jennifer Messerly, 2016-03-25 18:23:23]

Thanks guys.

https://codereview.chromium.org/1838473002/diff/20001/lib/transformer.dart
File lib/transformer.dart (right):

https://codereview.chromium.org/1838473002/diff/20001/lib/transformer.dart#ne...
lib/transformer.dart:49: files = new List<String>.from(value);
On 2016/03/25 17:36:29, Siggi Cherem (dart-lang) wrote:
> consider adding a TODO referring to
> https://github.com/dart-lang/collection/issues/25 (basically whenever we are
> cloning to make the type-system happy, we may want to convert the code to use
a
> type-coercion wrapper in the future)

IMO it's not really worth it. `new List.from` is already an explicit coercion,
and fairly concise. I'm not sure I agree with that feature request ... you're
either copying, or your doing an extra check on access, either way it's at least
O(N). In this case the data comes from a settings file so it won't be large
anyway.

https://codereview.chromium.org/1838473002/diff/20001/lib/transformer.dart#ne...
lib/transformer.dart:261: token?.type == TokenType.KEYWORD && token.lexeme ==
keyword.syntax;
On 2016/03/25 17:36:29, Siggi Cherem (dart-lang) wrote:
> was this broken?

no, but KeywordToken is private.

https://codereview.chromium.org/1838473002/diff/20001/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/1838473002/diff/20001/pubspec.yaml#newcode29
pubspec.yaml:29: sdk: ^1.8.0
On 2016/03/25 17:44:13, jakemac wrote:
> Does this sdk support the ^ syntax? I think it might be some 1.9 release

I hope so since it was already used (see Analyzer above). But I'm happy to bump
to 1.9 if you want to be safe :)

https://codereview.chromium.org/1838473002/diff/20001/test/observe_test_utils...
File test/observe_test_utils.dart (right):

https://codereview.chromium.org/1838473002/diff/20001/test/observe_test_utils...
test/observe_test_utils.dart:46: new List.from(changes.where((c) =>
c.indexChanged(index)));
On 2016/03/25 17:36:29, Siggi Cherem (dart-lang) wrote:
> I'm surprised that you need to replace `.toList` with `new List.from` here.
> Doesn't `toList()` preserve the type argument? 

they're different.

changes -- is probably a List<dynamic>
changes.toList() -- will give a List<dynamic> at runtime.

`new List.from` will infer as `new List<ListChangeRecord>.from()` which gives us
the explicit coercion we need to make this work.

https://codereview.chromium.org/1838473002/diff/20001/test/transformer_test.dart
File test/transformer_test.dart (right):

https://codereview.chromium.org/1838473002/diff/20001/test/transformer_test.d...
test/transformer_test.dart:129: Future _transform(String code) {
On 2016/03/25 17:36:29, Siggi Cherem (dart-lang) wrote:
> Doesn't matter here, but would we be able to keep String if we change should
> Chain.capture to have a generic type like:
> 
>     <T> static T capture(T callback(), ... );
> 
> ?

already filed https://github.com/dart-lang/stack_trace/issues/10 :)
but we're in code yellow. I'll take "good" over "perfect" :)

https://codereview.chromium.org/1838473002/diff/20001/test/transformer_test.d...
test/transformer_test.dart:137: await transformer.apply(transform);
On 2016/03/25 17:36:29, Siggi Cherem (dart-lang) wrote:
> is the change to use await necessary for now?
> 
> I mean, is this because we don't have union types and Future.then is
> practically:
> 
> class Future<T> {
>   <E> then({E | Future<E>} callback(T));
> }

yes it is necessary & that is the reason.

FWIW -- all of my changes are to make warnings/errors/hints go away.

[Jennifer Messerly, 2016-03-25 18:24:49]

Description was changed from

==========
fix observe package to be strong mode clean
==========

to

==========
fix observe package to be strong mode clean

R=sigmund@google.com

Committed:
https://github.com/dart-lang/observe/commit/fa9ac07ceaa44267fc4aacbede7c78c5e...
==========

[Jennifer Messerly, 2016-03-25 18:24:51]

Committed patchset #3 (id:40001) manually as
fa9ac07ceaa44267fc4aacbede7c78c5e908f347 (presubmit successful).

[Siggi Cherem (dart-lang), 2016-03-25 18:30:02]

https://codereview.chromium.org/1838473002/diff/20001/test/observe_test_utils...
File test/observe_test_utils.dart (right):

https://codereview.chromium.org/1838473002/diff/20001/test/observe_test_utils...
test/observe_test_utils.dart:46: new List.from(changes.where((c) =>
c.indexChanged(index)));
On 2016/03/25 18:23:23, John Messerly wrote:
> On 2016/03/25 17:36:29, Siggi Cherem (dart-lang) wrote:
> > I'm surprised that you need to replace `.toList` with `new List.from` here.
> > Doesn't `toList()` preserve the type argument? 
> 
> they're different.
> 
> changes -- is probably a List<dynamic>
> changes.toList() -- will give a List<dynamic> at runtime.
> 
> `new List.from` will infer as `new List<ListChangeRecord>.from()` which gives
us
> the explicit coercion we need to make this work.

Ah - sorry, for a moment I thought we inferred List.from from the argument, but
we are inferring it from the declared return type of this function. I can see it
now.

[Jennifer Messerly, 2016-03-25 18:31:28]

https://codereview.chromium.org/1838473002/diff/20001/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/1838473002/diff/20001/pubspec.yaml#newcode29
pubspec.yaml:29: sdk: ^1.8.0
On 2016/03/25 18:23:23, John Messerly wrote:
> On 2016/03/25 17:44:13, jakemac wrote:
> > Does this sdk support the ^ syntax? I think it might be some 1.9 release
> 
> I hope so since it was already used (see Analyzer above). But I'm happy to
bump
> to 1.9 if you want to be safe :)

oh, it seems maybe Pub just never allows it in SDK constraints. reverted this
part. rest of them seemed good.

[Jennifer Messerly, 2016-03-25 18:43:52]

On 2016/03/25 18:31:28, John Messerly wrote:
> https://codereview.chromium.org/1838473002/diff/20001/pubspec.yaml
> File pubspec.yaml (right):
> 
> https://codereview.chromium.org/1838473002/diff/20001/pubspec.yaml#newcode29
> pubspec.yaml:29: sdk: ^1.8.0
> On 2016/03/25 18:23:23, John Messerly wrote:
> > On 2016/03/25 17:44:13, jakemac wrote:
> > > Does this sdk support the ^ syntax? I think it might be some 1.9 release
> > 
> > I hope so since it was already used (see Analyzer above). But I'm happy to
> bump
> > to 1.9 if you want to be safe :)
> 
> oh, it seems maybe Pub just never allows it in SDK constraints. reverted this
> part. rest of them seemed good.

Hey Siggi/Jake can you try publishing? In theory I should be able to, but
publish is hanging on the "upload" step...