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

Add Iterable.fromIterator (or similar, name less important) #55961

Open
matanlurey opened this issue Jun 9, 2024 · 5 comments · May be fixed by #59908
Open

Add Iterable.fromIterator (or similar, name less important) #55961

matanlurey opened this issue Jun 9, 2024 · 5 comments · May be fixed by #59908
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

@matanlurey
Copy link
Contributor

Most of the time when creating a custom Iterator, the following pattern is followed:

final class Rect {
  /* ... */

  /// Every `(x, y)` coordinate that makes up this rectangle.
  Iterable<(int, int>) get positions => _PositionsIterable(this);
}

final class _PositionsIterable extends Iterable<(int, int)> {
  /* ... */

  @override
  Iterator<(int, int)> get iterator => _PositionsIterator(_rect);
}

final class _PositionsIterator implements Iterator<(int, int)> {
  /* ... */
}

While I can imagine this is useful for optimizations, it's boiler-plate for getting started.

I'd love to see the following factory method added:

final class Iterable<E> {
  factory Iterable.fromIterator(Iterator<E> Function() iterator) => /* ... */
}

That would mean being able to just write:

final class Rect {
  /* ... */

  /// Every `(x, y)` coordinate that makes up this rectangle.
  Iterable<(int, int>) get positions => Iterable.fromIterator(() => _PositionsIterator(this));
}

final class _PositionsIterator implements Iterator<(int, int)> {
  /* ... */
}

It's a relatively small improvement, but it also is a relatively small enhancement :)

@lrhn
Copy link
Member

lrhn commented Jun 9, 2024

As you noticed, its more like fromIteratorFactory. It would basically be:

abstract class Iterator<T> {  
  // ...
  Iterator.fromIteratorFactory(Iterator<T> Function() makeIterator) = _IterableFromIteratorFunction<T>;
  // ...
}

final class _IterableFromIteratorFactory<T> extends Iterable<T> {
  final Iterator<T> Function() _makeIterator;
  IterableFromIteratorFactory(this._makeIterator);
  Iterator<T> get iterator => _makeIterator();
}

Not unreasonable.

The reason I would very rarely use such a class is that I'd want to optimize some of the Iterable operations. If I can't, then there's rarely a reason to have an iterable at all.
Other people's use may differ.

@lrhn lrhn 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 Jun 9, 2024
@natebosch
Copy link
Member

Do you have a concrete example you can link to?

If sync* had equivalent performance would you prefer it over the separate Iterator class implementation?

Would iterable generator expressions solve your use case? dart-lang/language#1633

I do think this is worth adding. I hope we can find a shorter name than fromIteratorFactory.

@drxddy drxddy linked a pull request Jan 14, 2025 that will close this issue
@drxddy
Copy link

drxddy commented Jan 15, 2025

Hey @natebosch, @lrhn, I'm working on this, please assign this issue to me, I've submitted an initial pr #59908, there we can discuss if any changes are needed or if any additional test cases I need to cover

@lrhn
Copy link
Member

lrhn commented Jan 15, 2025

Code looks fine at a first glance.
I agree with @natebosch that the name is too long.

Maybe just fromIterables would be enough to signify that it's not just one Iterable.
Or withIterable where the singular word refers to the Iterable.iterator that the argument is giving an implementation of.

(I wish Dart had anonymous classes like Java, so you could just do

new Iterable{get iterator {…}}

rather than having to have all these "from functions" constructors.)

@drxddy
Copy link

drxddy commented Jan 15, 2025

Yeah, I couldn't find a better name for this

I think Iterable.withIterator makes sense and signifies the use case, anything else before I rename and push the changes?

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

Successfully merging a pull request may close this issue.

4 participants