Test equality argument order in iterable.

Test equality argument order in iterable.

R=lrn@google.com

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

[Lasse Reichstein Nielsen, 2013-06-26 08:26:59]

LGTM with comments.

https://codereview.chromium.org/17830002/diff/1/sdk/lib/_collection_dev/itera...
File sdk/lib/_collection_dev/iterable.dart (right):

https://codereview.chromium.org/17830002/diff/1/sdk/lib/_collection_dev/itera...
sdk/lib/_collection_dev/iterable.dart:697: if (e == element) return true;
Excellent!

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
File tests/corelib/list_contains_argument_order_test.dart (right):

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:1: import
"package:expect/expect.dart";
Add missing copyright header (remember to update year if copying it from a file
that doesn't say 2013).

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:14: throw new
Exception('Bad equality order.');
Change line to:
 Expect.fail("Bad equality order.");

You have imported expect.dart, might as well use it.

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:19: var list = <A>[new A()
];
Add space between '[' and 'new'.

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:20: var list2 = new
List<A>(1);
You can use cascade notation to initialize on the same line;
  var list2 = new List<A>(1)..[0] = new A();
  var list3 = new List<A>()..add(new A());

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:26: var iterable2 =
list.take(1);
If you want to do this for each iterable, do it inside the loop instead. I.e.,
remove these and inside the loop add:

  a = false;
  iterable.map((x) => x).contains(new B());
  Expect.isTrue(a);

  a = false;
  iterable.take(1).contains(new B());
  Expect.isTrue(a);

and then only have the base iterables in the "iterables" list.

If you don't like duplicate code (and who does?) make a helper function:
  test(iterable) {
    a = false
    Expect.isFalse(iterable.contains(new B()));
    Expect.isTrue(a);
  }
and inside the loop just do:
  test(iterable)
  test(iterable.map((x) => x));
  test(iterable.take(1));
... and it's easy to add more if you want it.

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:32: var iterable8 =
list4.take(1);
Try a Set, Map.keys, and Map.values as well:

  var iterable9 = new Set()..add(new A());
  var iterable10 = (new Map()..[new A()] = 0).keys;
  var iterable11 = (new Map()..[0] = new A()).values;

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:36: iterable4, iterable5,
iterable6, iterable7, iterable8];
Now that you can have each iterable created on a single line, you could do this
as one long literal, instead of having the intermediate variables:
  var iterables = [
      <A>[ new A() ],
      new List<A>(1)..[0] = new A(),
      ...
      (new Map()..[0] = new A()).values
  ];

Only change this if you think it's easier to read (but you will avoid a lot of
names!).

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:38: a = false;
Indentation is off-by-one (should be indented one more).

[zarah, 2013-06-26 09:08:34]

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
File tests/corelib/list_contains_argument_order_test.dart (right):

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:1: import
"package:expect/expect.dart";
On 2013/06/26 08:26:59, Lasse Reichstein Nielsen wrote:
> Add missing copyright header (remember to update year if copying it from a
file
> that doesn't say 2013).

Done.

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:14: throw new
Exception('Bad equality order.');
On 2013/06/26 08:26:59, Lasse Reichstein Nielsen wrote:
> Change line to:
>  Expect.fail("Bad equality order.");
> 
> You have imported expect.dart, might as well use it.

Done.

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:19: var list = <A>[new A()
];
On 2013/06/26 08:26:59, Lasse Reichstein Nielsen wrote:
> Add space between '[' and 'new'.

Done.

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:20: var list2 = new
List<A>(1);
On 2013/06/26 08:26:59, Lasse Reichstein Nielsen wrote:
> You can use cascade notation to initialize on the same line;
>   var list2 = new List<A>(1)..[0] = new A();
>   var list3 = new List<A>()..add(new A());

Done.

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:26: var iterable2 =
list.take(1);
On 2013/06/26 08:26:59, Lasse Reichstein Nielsen wrote:
> If you want to do this for each iterable, do it inside the loop instead. I.e.,
> remove these and inside the loop add:
> 
>   a = false;
>   iterable.map((x) => x).contains(new B());
>   Expect.isTrue(a);
> 
>   a = false;
>   iterable.take(1).contains(new B());
>   Expect.isTrue(a);
> 
> and then only have the base iterables in the "iterables" list.
> 
> If you don't like duplicate code (and who does?) make a helper function:
>   test(iterable) {
>     a = false
>     Expect.isFalse(iterable.contains(new B()));
>     Expect.isTrue(a);
>   }
> and inside the loop just do:
>   test(iterable)
>   test(iterable.map((x) => x));
>   test(iterable.take(1));
> ... and it's easy to add more if you want it.

Done.

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:32: var iterable8 =
list4.take(1);
On 2013/06/26 08:26:59, Lasse Reichstein Nielsen wrote:
> Try a Set, Map.keys, and Map.values as well:
> 
>   var iterable9 = new Set()..add(new A());
>   var iterable10 = (new Map()..[new A()] = 0).keys;
>   var iterable11 = (new Map()..[0] = new A()).values;

Done.

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:36: iterable4, iterable5,
iterable6, iterable7, iterable8];
On 2013/06/26 08:26:59, Lasse Reichstein Nielsen wrote:
> Now that you can have each iterable created on a single line, you could do
this
> as one long literal, instead of having the intermediate variables:
>   var iterables = [
>       <A>[ new A() ],
>       new List<A>(1)..[0] = new A(),
>       ...
>       (new Map()..[0] = new A()).values
>   ];
> 
> Only change this if you think it's easier to read (but you will avoid a lot of
> names!).

Done.

https://codereview.chromium.org/17830002/diff/1/tests/corelib/list_contains_a...
tests/corelib/list_contains_argument_order_test.dart:38: a = false;
On 2013/06/26 08:26:59, Lasse Reichstein Nielsen wrote:
> Indentation is off-by-one (should be indented one more).

Done.

[zarah, 2013-06-26 09:14:32]

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