Fixed strong mode errors and warnings reachable from lib/observe.dart

Fixed strong mode errors and warnings reachable from lib/observe.dart

R=jmesserly@google.com, rnystrom@google.com

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

[vsm, 2016-01-22 16:43:18]

vsm@google.com changed reviewers:
+ jmesserly@google.com, rnystrom@google.com

[vsm, 2016-01-22 16:47:15]

The removes errors and warnings reachable from lib/observe.dart and
lib/html.dart.

There are still issues in lib/transformer.dart (where the better fix is
upstream) and test/*.

https://codereview.chromium.org/1616953004/diff/20001/lib/src/list_path_obser...
File lib/src/list_path_observer.dart (right):

https://codereview.chromium.org/1616953004/diff/20001/lib/src/list_path_obser...
lib/src/list_path_observer.dart:48: _value = notifyPropertyChange(#value,
_value, newValue) as Iterable<P>;
Perhaps we need to be more permissive on casts from dynamic.  Or provide a
static cast?

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

https://codereview.chromium.org/1616953004/diff/20001/lib/src/observable_box....
lib/src/observable_box.dart:23: _value = notifyPropertyChange(#value, _value,
newValue) as T;
Another cast from dynamic.

[Jennifer Messerly, 2016-01-22 17:45:01]

LGTM

https://codereview.chromium.org/1616953004/diff/20001/lib/src/list_path_obser...
File lib/src/list_path_observer.dart (right):

https://codereview.chromium.org/1616953004/diff/20001/lib/src/list_path_obser...
lib/src/list_path_observer.dart:48: _value = notifyPropertyChange(#value,
_value, newValue) as Iterable<P>;
On 2016/01/22 16:47:15, vsm wrote:
> Perhaps we need to be more permissive on casts from dynamic.  Or provide a
> static cast?

generic methods FTW!

    notifyPropertyChange<T>(Symbol name, T oldValue, T newValue)

The problem is, I'm not sure typing is right on _observers. I think it should be
PathObserver<P>, but not sure PathObserver is generic.

The easy fix here though is probably this:

    var newValue = _observers.map((o) => o.value as P);
    notifyPropertyChange(#value, _value, newValue);
    _value = newValue;

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

https://codereview.chromium.org/1616953004/diff/20001/lib/src/observable_box....
lib/src/observable_box.dart:23: _value = notifyPropertyChange(#value, _value,
newValue) as T;
On 2016/01/22 16:47:15, vsm wrote:
> Another cast from dynamic.

this one too could be either generic method for notifyPropertyChange, or:

notifyPropertyChange(#value, _value, newValue);
_value = newValue;


The return value from nPC is just a convenience. :)

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

https://codereview.chromium.org/1616953004/diff/20001/lib/transformer.dart#ne...
lib/transformer.dart:398: '__\$$name = notifyPropertyChange(#$name, __\$$name,
value); }');
this is where the generic notifyPropertyChange would help, it means the
generated code would also be well typed

[vsm, 2016-01-22 18:49:22]

PTAL

https://codereview.chromium.org/1616953004/diff/20001/lib/src/list_path_obser...
File lib/src/list_path_observer.dart (right):

https://codereview.chromium.org/1616953004/diff/20001/lib/src/list_path_obser...
lib/src/list_path_observer.dart:48: _value = notifyPropertyChange(#value,
_value, newValue) as Iterable<P>;
On 2016/01/22 17:45:00, John Messerly wrote:
> On 2016/01/22 16:47:15, vsm wrote:
> > Perhaps we need to be more permissive on casts from dynamic.  Or provide a
> > static cast?
> 
> generic methods FTW!
> 
>     notifyPropertyChange<T>(Symbol name, T oldValue, T newValue)
> 
> The problem is, I'm not sure typing is right on _observers. I think it should
be
> PathObserver<P>, but not sure PathObserver is generic.
> 
> The easy fix here though is probably this:
> 
>     var newValue = _observers.map((o) => o.value as P);
>     notifyPropertyChange(#value, _value, newValue);
>     _value = newValue;
> 

Done.

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

https://codereview.chromium.org/1616953004/diff/20001/lib/src/observable_box....
lib/src/observable_box.dart:23: _value = notifyPropertyChange(#value, _value,
newValue) as T;
On 2016/01/22 17:45:00, John Messerly wrote:
> On 2016/01/22 16:47:15, vsm wrote:
> > Another cast from dynamic.
> 
> this one too could be either generic method for notifyPropertyChange, or:
> 
> notifyPropertyChange(#value, _value, newValue);
> _value = newValue;
> 
> 
> The return value from nPC is just a convenience. :)

Done.

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

https://codereview.chromium.org/1616953004/diff/20001/lib/transformer.dart#ne...
lib/transformer.dart:398: '__\$$name = notifyPropertyChange(#$name, __\$$name,
value); }');
On 2016/01/22 17:45:00, John Messerly wrote:
> this is where the generic notifyPropertyChange would help, it means the
> generated code would also be well typed

Acknowledged.

[Bob Nystrom, 2016-01-22 18:50:25]

LGTM.

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

https://codereview.chromium.org/1616953004/diff/20001/lib/src/list_diff.dart#...
lib/src/list_diff.dart:219: } else if (oldStart == oldEnd)
Braces around the else.

https://codereview.chromium.org/1616953004/diff/20001/lib/src/list_diff.dart#...
lib/src/list_diff.dart:376: List list, List<ListChangeRecord> records) {
List -> List<Object>?

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

https://codereview.chromium.org/1616953004/diff/20001/lib/src/observable_list...
lib/src/observable_list.dart:116: var len = iterable.length;
Nit: "len" -> "length"

since you're in here anyway.

https://codereview.chromium.org/1616953004/diff/20001/lib/src/observable_list...
lib/src/observable_list.dart:119: addedCount: len, removed:
_list.getRange(index, len).toList()));
"getRange(index, len).toList()" -> "sublist(index, len)"

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

https://codereview.chromium.org/1616953004/diff/20001/lib/src/path_observer.d...
lib/src/path_observer.dart:475: List parse(String path) {
So the old return type was straight up wrong, right?

How about List<Object> here and on the keys field?

[vsm, 2016-01-22 19:05:06]

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

https://codereview.chromium.org/1616953004/diff/20001/lib/src/list_diff.dart#...
lib/src/list_diff.dart:219: } else if (oldStart == oldEnd)
On 2016/01/22 18:50:25, Bob Nystrom wrote:
> Braces around the else.

Done.

https://codereview.chromium.org/1616953004/diff/20001/lib/src/list_diff.dart#...
lib/src/list_diff.dart:376: List list, List<ListChangeRecord> records) {
On 2016/01/22 18:50:25, Bob Nystrom wrote:
> List -> List<Object>?

Done.

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

https://codereview.chromium.org/1616953004/diff/20001/lib/src/observable_list...
lib/src/observable_list.dart:116: var len = iterable.length;
On 2016/01/22 18:50:25, Bob Nystrom wrote:
> Nit: "len" -> "length"
> 
> since you're in here anyway.

Done.

https://codereview.chromium.org/1616953004/diff/20001/lib/src/observable_list...
lib/src/observable_list.dart:119: addedCount: len, removed:
_list.getRange(index, len).toList()));
On 2016/01/22 18:50:25, Bob Nystrom wrote:
> "getRange(index, len).toList()" -> "sublist(index, len)"

Done.

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

https://codereview.chromium.org/1616953004/diff/20001/lib/src/path_observer.d...
lib/src/path_observer.dart:475: List parse(String path) {
On 2016/01/22 18:50:25, Bob Nystrom wrote:
> So the old return type was straight up wrong, right?
> 
> How about List<Object> here and on the keys field?

Yes, the list has Symbols and Strings (at least statically).

Done.

[Jennifer Messerly, 2016-01-22 19:40:37]

lgtm

https://codereview.chromium.org/1616953004/diff/80001/lib/src/list_path_obser...
File lib/src/list_path_observer.dart (right):

https://codereview.chromium.org/1616953004/diff/80001/lib/src/list_path_obser...
lib/src/list_path_observer.dart:48: _value = notifyPropertyChange
/*<Iterable<P>>*/ (#value, _value, newValue);
Hmmm. Both newValue and _value should be Iterable<P> ... I'd expect we could
infer this one?

[vsm, 2016-01-22 20:35:16]

https://codereview.chromium.org/1616953004/diff/80001/lib/src/list_path_obser...
File lib/src/list_path_observer.dart (right):

https://codereview.chromium.org/1616953004/diff/80001/lib/src/list_path_obser...
lib/src/list_path_observer.dart:48: _value = notifyPropertyChange
/*<Iterable<P>>*/ (#value, _value, newValue);
On 2016/01/22 19:40:37, John Messerly wrote:
> Hmmm. Both newValue and _value should be Iterable<P> ... I'd expect we could
> infer this one?

Yes, removed the explicit type param.

[vsm, 2016-01-22 20:36:09]

Description was changed from

==========
Fixed strong mode errors and warnings reachable from lib/observe.dart
==========

to

==========
Fixed strong mode errors and warnings reachable from lib/observe.dart

R=jmesserly@google.com, rnystrom@google.com

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

[vsm, 2016-01-22 20:36:10]

Committed patchset #6 (id:100001) manually as
671802a0e86b9cef5454d2f31893e5f4333371b5 (presubmit successful).