Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addition to stdlib proposal: Iterable.enumerate #32467

Open
phrohdoh opened this issue Mar 8, 2018 · 13 comments
Open

Addition to stdlib proposal: Iterable.enumerate #32467

phrohdoh opened this issue Mar 8, 2018 · 13 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-enhancement A request for a change that isn't a bug

Comments

@phrohdoh
Copy link

phrohdoh commented Mar 8, 2018

The iterator returned yields pairs (i, e), where i is the current index of iteration and e is the element returned by the iterator.

The enumerate adaptor is exactly the same as map but with the added index parameter.

enumerate(T f(I i, E e)) → Iterable
Returns a new lazy Iterable with elements that are created by calling f on each element of this Iterable in iteration order.

Here is one such implementation:

https://github.com/google/quiver-dart/blob/master/lib/src/iterables/enumerate.dart

@phrohdoh phrohdoh changed the title Language Proposal: Iterable.enumerate Addition to stdlib proposal: Iterable.enumerate Mar 8, 2018
@phrohdoh
Copy link
Author

phrohdoh commented Mar 8, 2018

One of the many use-cases for this addition is as follows.

    children: photos.enumerate((index, photo) {
      return new Positioned(
        left: 6.0 * index,
        height: 60.0,
        child: new Image.file(new File(photo), fit: BoxFit.cover),
      );
    }).toList(),

@acidjazz
Copy link

acidjazz commented Mar 8, 2018

It would be awesome to have the index w/in an iterating function of a list and be able to avoid dangerous methods like using .indexOf() inside

@natebosch
Copy link
Member

This reminds me of Map.entries

What about something like:

class ListEntry<E> {
  E element;
  int index;
}

class Iterable<E> {
  ...
  Iterable<ListEntry<E>> get entries;
}

Then your example is:

children: photos.entries.map((entry) {
  return new Positioned(
    left: 6.0 * entry.index,
    height: 60.0,
    child: new Image.file(new File(entry.element), fit: BoxFit.cover),
  );
}).toList();

@phrohdoh
Copy link
Author

phrohdoh commented Mar 8, 2018

Oh that is interesting.

I did not know entries existed but unfortunately that is a method on Map not Iterable which means you have to do photos.asMap().entries.map(...).

I don't know if that requirement is a hurdle in practice.

@acidjazz
Copy link

acidjazz commented Mar 9, 2018

@natebosch excellent work around until this gets added, but still I think it'd be simpler to have this functionality

@phrohdoh
Copy link
Author

phrohdoh commented Mar 9, 2018

Thinking about this further I don't think Map.entries covers all cases.

Namely the cases where you do not have the entire collection up-front (networking, etc).

Nonetheless it is useable for the presented situation.

@jonahwilliams
Copy link
Contributor

Is the use case for this so compelling that you couldn't just use a for loop instead? They're just about the same length and complexity.

var children = <Widget>[];
for (var i = 0; i < photos.length; i++) {
  children.add(new Positioned(
    left: 6.0 * i,
    height: 60.0,
    child: new Image.file(new File(photos[i]), fit: BoxFit.cover),
  ));
}

There are so many different operations we do on Lists which could be expressed as their own Iterator method. It seems like it would be more sustainable long term to be less opinionated, and spend the effort on something like extension methods.

Then you could have libraries like quiver define their own Iterator methods.

@phrohdoh
Copy link
Author

phrohdoh commented Mar 9, 2018

There are many things you can do with lazy combinator chains that don't work out to as neat of a solution as a good old-fashioned for loop.

The example I provided was intentionally small and simple.

Being opinionated about the stdlib provided by your language is not a bad thing nor is it mutually exclusive with extension methods.

Of course time isn't infinite so you must pick your battles but is the addition of extension methods on the roadmap for Dart currently?

The same argument could have been made for map FWIW.

@matanlurey
Copy link
Contributor

The reality is adding more members on a core collection like this without extension methods is extremely time consuming and wouldn't have large enough value to justify it. I'd recommend using a top-level method for the time being:

Iterable<E> mapEnumerated<T>(Iterable<T> iterable, E Function<E>(T, int) fn) sync* {
  var index = 0;
  for (final item in iterable) {
    yield fn(item, index++);
  }
}

Example use:

children: mapEnumerated(photos, (element, index) {
  return new Positioned(
    left: 6.0 * index,
    height: 60.0,
    child: new Image.file(new File(element), fit: BoxFit.cover),
  );
}).toList();

Compared to photos.mapEnumerated, its not that much more typing.

@dgrove dgrove added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-enhancement A request for a change that isn't a bug labels Mar 16, 2018
@jarrodcolburn
Copy link

jarrodcolburn commented Jan 22, 2019

How about adding zero new methods and simply taking advantage of optional parameters? The map method is great :) and map already expects a callback with one argument (the element). Why not change the map callback to allow for an optional index param.
Iterable.map

// currently... (E e)
Iterable<T> map<T>(T f(E e)) => new MappedIterable<E, T>(this, f);

// proposed... (E e, [int index])
Iterable<T> map<T>(T f(E e, [int index])) => new MappedIterable<E, T>(this, f);
// added `index` as optional ^ positional argument to callback

Since the index is optional, it should limit breaking changes with exiting code.

@natebosch
Copy link
Member

Since the index is optional, it should limit breaking changes with exiting code

It's breaking for any class which implements Iterable since it won't have a matching signature on it's map. Those are fairly widespread.

@jarrodcolburn
Copy link

jarrodcolburn commented Jan 22, 2019

Since the index is optional, it should limit breaking changes with exiting code

It's breaking for any class which implements Iterable since it won't have a matching signature on it's map. Those are fairly widespread.

There's no way for Dart to understand that params (E e) is a subset of (E e, [int index)) ? Or at least that it satisfies the former's condition.

@natebosch
Copy link
Member

natebosch commented Jan 22, 2019

There's no way for Dart to understand that (E e) is a subset of (E e, [int index)) ?

Ah I misread the proposal. The subclass wouldn't be a static error in this case, but existing callers would have a static error since they must provide a callback with an optional argument. There is no way to express with the Dart type system "Either a function that takes 1 argument of type E, or a function that takes an E and an int". You have to fall back to Function and then check at runtime what it's arguments are - doing that would cause all subclasses to have static errors.

You can play around with the analyzer to see these errors in practice.
https://dartpad.dartlang.org/3fd1fb897eaa174630ff9cd4b4672cad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants