Make Iterable more powerful (and lazy).

Make Iterable more powerful (and lazy).

[floitsch, 2012-11-06 17:41:43]

First draft of the lazy iterable conversion.
Patch-set 1 only moves functions. (so if you want to see what has changed, just
diff to patch-set 1).

[Anders Johnsen, 2012-11-06 17:51:47]

LGTM,

Maybe List.from(Collection<E> other) should be List.from(Iterable<E> other) now?

https://codereview.chromium.org/11366111/diff/4/sdk/lib/core/iterable.dart
File sdk/lib/core/iterable.dart (right):

https://codereview.chromium.org/11366111/diff/4/sdk/lib/core/iterable.dart#ne...
sdk/lib/core/iterable.dart:12: abstract class Iterable<E> {
I think this would be a good time to introduce

 Iterable expand(Iterable f(E element));

[Mads Ager (google), 2012-11-07 07:13:19]

LGTM

Are you going to put this in on the branch and then fix the various
implementations in follow up changes?

https://codereview.chromium.org/11366111/diff/4/sdk/lib/core/iterable.dart
File sdk/lib/core/iterable.dart (right):

https://codereview.chromium.org/11366111/diff/4/sdk/lib/core/iterable.dart#ne...
sdk/lib/core/iterable.dart:73: Dynamic combine(var previousValue, E element)) {
Dynamic -> dynamic

[Lasse Reichstein Nielsen, 2012-11-12 09:38:03]

STV

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart
File sdk/lib/core/iterable.dart (right):

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart...
sdk/lib/core/iterable.dart:21: Queryable<E> query();
Make it a getter. Even if it returns a new Queryable every time, they are for
all purposes equivalent.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart...
sdk/lib/core/iterable.dart:36: * Returns a lazy [Queryable] with all elements
[:e:] that satisfy the
You don't need to name 'e' if you don't use it later. Either remove "[:e:]" or
change to "predicate [:f(e):]".

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart...
sdk/lib/core/iterable.dart:82: }
You often want a reduction that uses the first value as initial and then
iterates on the remaining. It'll have to throw on empty iterables, and I don't
have a good name for it, but I think we should have it.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart...
sdk/lib/core/iterable.dart:115: bool get isEmpty => !iterator().hasNext;
Consider adding 'hasElements' that has the opposite value. It's often useful and
more readable.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterator.dart
File sdk/lib/core/iterator.dart (right):

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterator.dart...
sdk/lib/core/iterator.dart:36: bool hasNext;
Change to 'T get current'?

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/list.dart
File sdk/lib/core/list.dart (right):

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:55: * Alias for [add].
Remove it.
It's slightly misnamed (other operations named 'last' refer to the current last
element, this one adds a new one) and redundant.
Adding at the end is what lists are all about.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:134: List<E> get view;
I still don't think 'view' signifies something inherently read-only. A view is
just something backed by something else. There is nothing preventing a "view"
from being write-through. I would prefer 'asUnmodifiable'

Please use "unmodifiable" consistently about collections that can't be modified.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dart
File sdk/lib/core/queryable.dart (right):

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:5: abstract class Queryable<E> extends Iterable<E> {
Needs a description on the class. What is it?

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:19: * that moment on no other elements are passed
through.
Is the first non-matching element included?
(I assume not, but it's not clear).

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:43: * handy.
We don't have an easy way to test if an object is a Type, so this would require
mirroring.
For now: No.
I wouldn't mind if Type had a "isInstance(Object v)". Perhaps a Type could be a
Type<T> for the type (but maybe I'm being too Java'ish).

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:66: Queryable<E> firstMatching(bool f(E element));
Why is this a Queryable if it only returns one element?
Shouldn't it just eagerly return the value?

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:72: Queryable<E> firstMatchingOrDefault(bool f(E
element), E defaultValue);
Make the defaultValue a closure returning the value, then make it optional (and
don't provide a default it if omitted), and rename to firstMatching (and remove
the one above).

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:81: Queryable<E> lastMatching(bool f(E element));
Why not eager and returning a value?
This queryable can't return a value until the entire source has been iterated
anyway.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:87: Queryable<E> lastMatchingOrDefault(bool f(E
element), E defaultValue);
As above, make defaultValue an optional function that returns the default value.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:91: * satisfying [f]. If no or more than one element
match, then an exception
"If no element match, or if more than one element match, then ..."
Is it an exception, or an error? If it's an exception, you should say which one.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:100: * an error. This method is thus not equivalent
to
Have you considered which error? Seems like a RangeError.

[floitsch, 2012-11-12 10:23:49]

PTAL.

[Lasse Reichstein Nielsen, 2012-11-14 14:09:02]

On 2012/11/12 10:23:49, floitsch wrote:
> PTAL.

LGTM with comments still stands :)

[Lasse Reichstein Nielsen, 2012-11-14 14:09:11]

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/collection.dart
File sdk/lib/core/collection.dart (right):

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:12: * Might throw an [UnsupportedError] if [this]
is not extendable.
"extendable"? I think "extensible" is more common.

[floitsch, 2012-11-16 22:31:07]

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/collection.dart
File sdk/lib/core/collection.dart (right):

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:12: * Might throw an [UnsupportedError] if [this]
is not extendable.
On 2012/11/14 14:09:12, Lasse Reichstein Nielsen wrote:
> "extendable"? I think "extensible" is more common.

Done.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart
File sdk/lib/core/iterable.dart (right):

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart...
sdk/lib/core/iterable.dart:21: Queryable<E> query();
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> Make it a getter. Even if it returns a new Queryable every time, they are for
> all purposes equivalent.

the name is a verb. -> no getter. But if we change the name it can become a
getter.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart...
sdk/lib/core/iterable.dart:36: * Returns a lazy [Queryable] with all elements
[:e:] that satisfy the
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> You don't need to name 'e' if you don't use it later. Either remove "[:e:]" or
> change to "predicate [:f(e):]".

Done.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart...
sdk/lib/core/iterable.dart:82: }
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> You often want a reduction that uses the first value as initial and then
> iterates on the remaining. It'll have to throw on empty iterables, and I don't
> have a good name for it, but I think we should have it.

if we find a good way, why not.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart...
sdk/lib/core/iterable.dart:115: bool get isEmpty => !iterator().hasNext;
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> Consider adding 'hasElements' that has the opposite value. It's often useful
and
> more readable.

hasElements is imho a bad name. I would prefer isNotEmpty. That's what we use in
real world when we speak of collections.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterator.dart
File sdk/lib/core/iterator.dart (right):

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterator.dart...
sdk/lib/core/iterator.dart:36: bool hasNext;
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> Change to 'T get current'?

Done.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dart
File sdk/lib/core/queryable.dart (right):

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:5: abstract class Queryable<E> extends Iterable<E> {
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> Needs a description on the class. What is it?

Done.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:19: * that moment on no other elements are passed
through.
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> Is the first non-matching element included?
> (I assume not, but it's not clear).

Yes, but the comments need to be improved by a lot. It is difficult to express
an iterable in terms of the behavior of their iterators.
Also it needs to be clear that no further elements will be iterated over.
Slightly improved the doc.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:66: Queryable<E> firstMatching(bool f(E element));
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> Why is this a Queryable if it only returns one element?
> Shouldn't it just eagerly return the value?

doh. yes.
fixed and all the others below.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:72: Queryable<E> firstMatchingOrDefault(bool f(E
element), E defaultValue);
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> Make the defaultValue a closure returning the value, then make it optional
(and
> don't provide a default it if omitted), and rename to firstMatching (and
remove
> the one above).

Done.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:81: Queryable<E> lastMatching(bool f(E element));
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> Why not eager and returning a value?
> This queryable can't return a value until the entire source has been iterated
> anyway.

Done.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:87: Queryable<E> lastMatchingOrDefault(bool f(E
element), E defaultValue);
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> As above, make defaultValue an optional function that returns the default
value.

Done.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:91: * satisfying [f]. If no or more than one element
match, then an exception
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> "If no element match, or if more than one element match, then ..."
> Is it an exception, or an error? If it's an exception, you should say which
one.

added TODO.

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/queryable.dar...
sdk/lib/core/queryable.dart:100: * an error. This method is thus not equivalent
to
On 2012/11/12 09:38:03, Lasse Reichstein Nielsen wrote:
> Have you considered which error? Seems like a RangeError.

No. Added TODO.

[Lasse Reichstein Nielsen, 2012-11-19 13:36:08]

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart
File sdk/lib/core/iterable.dart (right):

https://codereview.chromium.org/11366111/diff/4013/sdk/lib/core/iterable.dart...
sdk/lib/core/iterable.dart:21: Queryable<E> query();
But it IS a noun ?!? :)