Finish @nex3's strong-mode fixes in package:collection and test it in Travis

Finish @nex3's strong-mode fixes in package:collection and test it in Travis

Example Travis run: https://travis-ci.org/ochafik/collection/builds/116790351
Fixes https://github.com/dart-lang/collection/issues/23

Corresponding branch: https://github.com/dart-lang/collection/compare/master...ochafik:strong-mode

BUG=

[ochafik, 2016-03-18 10:56:07]

Description was changed from

==========
Finish @nex3's strong-mode fixes and test it in Travis

BUG=
==========

to

==========
Finish @nex3's strong-mode fixes and test it in Travis

Example Travis run: https://travis-ci.org/ochafik/collection/builds/116790351

BUG=
==========

[ochafik, 2016-03-18 10:56:33]

Description was changed from

==========
Finish @nex3's strong-mode fixes and test it in Travis

Example Travis run: https://travis-ci.org/ochafik/collection/builds/116790351

BUG=
==========

to

==========
Finish @nex3's strong-mode fixes and test it in Travis

Example Travis run: https://travis-ci.org/ochafik/collection/builds/116790351
Fixes https://github.com/dart-lang/collection/issues/23

BUG=
==========

[ochafik, 2016-03-18 10:57:01]

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

[ochafik, 2016-03-18 10:58:04]

ochafik@google.com changed reviewers:
+ leafp@google.com

[ochafik, 2016-03-18 10:58:25]

Description was changed from

==========
Finish @nex3's strong-mode fixes and test it in Travis

Example Travis run: https://travis-ci.org/ochafik/collection/builds/116790351
Fixes https://github.com/dart-lang/collection/issues/23

BUG=
==========

to

==========
Finish @nex3's strong-mode fixes and test it in Travis

Example Travis run: https://travis-ci.org/ochafik/collection/builds/116790351
Fixes https://github.com/dart-lang/collection/issues/23

Corresponding branch:
https://github.com/dart-lang/collection/compare/master...ochafik:strong-mode-...

BUG=
==========

[ochafik, 2016-03-18 11:46:00]

Description was changed from

==========
Finish @nex3's strong-mode fixes and test it in Travis

Example Travis run: https://travis-ci.org/ochafik/collection/builds/116790351
Fixes https://github.com/dart-lang/collection/issues/23

Corresponding branch:
https://github.com/dart-lang/collection/compare/master...ochafik:strong-mode-...

BUG=
==========

to

==========
Finish @nex3's strong-mode fixes and test it in Travis

Example Travis run: https://travis-ci.org/ochafik/collection/builds/116790351
Fixes https://github.com/dart-lang/collection/issues/23

Corresponding branch:
https://github.com/dart-lang/collection/compare/master...ochafik:strong-mode

BUG=
==========

[ochafik, 2016-03-18 11:47:42]

Description was changed from

==========
Finish @nex3's strong-mode fixes and test it in Travis

Example Travis run: https://travis-ci.org/ochafik/collection/builds/116790351
Fixes https://github.com/dart-lang/collection/issues/23

Corresponding branch:
https://github.com/dart-lang/collection/compare/master...ochafik:strong-mode

BUG=
==========

to

==========
Finish @nex3's strong-mode fixes in package:collection and test it in Travis

Example Travis run: https://travis-ci.org/ochafik/collection/builds/116790351
Fixes https://github.com/dart-lang/collection/issues/23

Corresponding branch:
https://github.com/dart-lang/collection/compare/master...ochafik:strong-mode

BUG=
==========

[Leaf, 2016-03-18 16:35:40]

https://codereview.chromium.org/1817463002/diff/1/lib/src/algorithms.dart
File lib/src/algorithms.dart (right):

https://codereview.chromium.org/1817463002/diff/1/lib/src/algorithms.dart#new...
lib/src/algorithms.dart:38: return _comparableBinarySearch/*<T>*/(sortedList,
value);
Does inference not figure out this type argument?  Fine with leaving it, just
surprised if it's not caught.

https://codereview.chromium.org/1817463002/diff/1/lib/src/equality.dart
File lib/src/equality.dart (right):

https://codereview.chromium.org/1817463002/diff/1/lib/src/equality.dart#newco...
lib/src/equality.dart:37: bool equals(E e1, E e2) => e1 == e2;
I think these could safely be left as Object for backwards compatibility, if
desired.

https://codereview.chromium.org/1817463002/diff/1/lib/src/unmodifiable_wrappe...
File lib/src/unmodifiable_wrappers.dart (right):

https://codereview.chromium.org/1817463002/diff/1/lib/src/unmodifiable_wrappe...
lib/src/unmodifiable_wrappers.dart:28: static final _error = new
UnsupportedError(
Another way to tackle this is to define 

 static /*=T*/ _throw/*<T>*/() { ...}

and then call it as

void set length(int newLength) => _throw();

inference will infer bottom for T, and it should all work out.

[ochafik, 2016-03-19 01:38:33]

Thanks Leaf!

https://codereview.chromium.org/1817463002/diff/1/lib/src/algorithms.dart
File lib/src/algorithms.dart (right):

https://codereview.chromium.org/1817463002/diff/1/lib/src/algorithms.dart#new...
lib/src/algorithms.dart:38: return _comparableBinarySearch/*<T>*/(sortedList,
value);
On 2016/03/18 16:35:40, Leaf wrote:
> Does inference not figure out this type argument?  Fine with leaving it, just
> surprised if it's not caught.

It does, thanks!

https://codereview.chromium.org/1817463002/diff/1/lib/src/equality.dart
File lib/src/equality.dart (right):

https://codereview.chromium.org/1817463002/diff/1/lib/src/equality.dart#newco...
lib/src/equality.dart:37: bool equals(E e1, E e2) => e1 == e2;
On 2016/03/18 16:35:40, Leaf wrote:
> I think these could safely be left as Object for backwards compatibility, if
> desired.

Any potential backwards-incompatibility would only affect strong-mode-clean
code, right? (which there isn't much of yet?)

I vote for a package version bump (to stay on safe side) and cleaner / more
generic-intensive code...

Natalie, WDYT?

https://codereview.chromium.org/1817463002/diff/1/lib/src/unmodifiable_wrappe...
File lib/src/unmodifiable_wrappers.dart (right):

https://codereview.chromium.org/1817463002/diff/1/lib/src/unmodifiable_wrappe...
lib/src/unmodifiable_wrappers.dart:28: static final _error = new
UnsupportedError(
On 2016/03/18 16:35:40, Leaf wrote:
> Another way to tackle this is to define 
> 
>  static /*=T*/ _throw/*<T>*/() { ...}
> 
> and then call it as
> 
> void set length(int newLength) => _throw();
> 
> inference will infer bottom for T, and it should all work out.

Like it, thanks!

[nweiz, 2016-03-21 20:09:18]

https://codereview.chromium.org/1817463002/diff/1/lib/src/equality.dart
File lib/src/equality.dart (right):

https://codereview.chromium.org/1817463002/diff/1/lib/src/equality.dart#newco...
lib/src/equality.dart:37: bool equals(E e1, E e2) => e1 == e2;
On 2016/03/19 01:38:33, ochafik wrote:
> On 2016/03/18 16:35:40, Leaf wrote:
> > I think these could safely be left as Object for backwards compatibility, if
> > desired.
> 
> Any potential backwards-incompatibility would only affect strong-mode-clean
> code, right? (which there isn't much of yet?)
> 
> I vote for a package version bump (to stay on safe side) and cleaner / more
> generic-intensive code...
> 
> Natalie, WDYT?

I'm not sure I see the backwards-incompatibility here. Won't code that doesn't
specify a type argument default to using DefaultEquality<Object> anyway?

I'd strongly like to avoid a major version bump for this package. It's very
widely used, and changing the major version will create a lot of painful ripples
through the ecosystem. I also don't think we need to worry about
backwards-incompatibilities that only affect strong mode, at least until a
package is officially strong-mode-clean.

https://codereview.chromium.org/1817463002/diff/20001/.travis.yml
File .travis.yml (right):

https://codereview.chromium.org/1817463002/diff/20001/.travis.yml#newcode1
.travis.yml:1: language: dart
Historically, we've mostly tested Google packages on our own package bots rather
than Travis. I feel a little weird about putting this package on Travis without
a broader plan.

https://codereview.chromium.org/1817463002/diff/20001/.travis.yml#newcode6
.travis.yml:6: - dartanalyzer --strong `find lib -name '*.dart'`
Also add a .analysis_options file that enables strong mode.

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

https://codereview.chromium.org/1817463002/diff/20001/lib/src/iterable_zip.da...
lib/src/iterable_zip.dart:5: import "dart:collection" show IterableBase;
Generally we don't use show/hide with import unless we know there'll be a
conflict otherwise.

https://codereview.chromium.org/1817463002/diff/20001/lib/src/unmodifiable_wr...
File lib/src/unmodifiable_wrappers.dart (right):

https://codereview.chromium.org/1817463002/diff/20001/lib/src/unmodifiable_wr...
lib/src/unmodifiable_wrappers.dart:28: static /*=T*/ _throw/*<T>*/() {
Haha, that's clever.

[nweiz, 2016-03-24 21:40:18]

Since Olivier's on vacation, I'm going to take over this CL at
https://codereview.chromium.org/1831103004.

[Leaf, 2016-03-24 22:27:20]

lgtm

[ochafik, 2016-03-25 10:48:22]

https://codereview.chromium.org/1817463002/diff/20001/.travis.yml
File .travis.yml (right):

https://codereview.chromium.org/1817463002/diff/20001/.travis.yml#newcode1
.travis.yml:1: language: dart
On 2016/03/21 20:09:18, nweiz wrote:
> Historically, we've mostly tested Google packages on our own package bots
rather
> than Travis. I feel a little weird about putting this package on Travis
without
> a broader plan.

For DDC, Travis proved to be quite handy and incredibly well integrated to
github (helps a lot for external PRs, for instance).