Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

separated method added in Iterable extension #343

Conversation

kishan-dhankecha
Copy link

@kishan-dhankecha kishan-dhankecha commented May 20, 2024

Fixes dart-lang/core#674.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.

@kishan-dhankecha kishan-dhankecha marked this pull request as ready for review May 20, 2024 12:03
@kishan-dhankecha kishan-dhankecha changed the title separated method added in Iterable extension separated method added in Iterable extension May 20, 2024
@kishan-dhankecha
Copy link
Author

@kevmoo can you please review this one?

@kevmoo
Copy link
Member

kevmoo commented May 31, 2024

I'd leave the review to @devoncarew and/or @lrhn

Copy link
Member

@kevmoo kevmoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM – just want one more nod on this. We should be mindful when adding things we'll need to support "forever".

yield separator;
yield iterator.current;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this version is flexible enough.
The use-cases I've heard mentioned won't all be satisfies by inserting the same value every time, and the typing might not work out.

I'd at least want a version where the separator is generated each time.
Then it could be generated based on position, or on adjacent values.

Something like these would cover those:

Iterable<T> separatedByValue(T separator) { ... }
Iterable<T> separatedBy(T Function() separator) { ... }

and possibly also:

// Position is index of next element, from 1 to `lenght` - 1.
Iterable<T> separatedByIndex(T Function(int position) separator) { ... }
Iterable<T> separatedByElements(T Function(T before, T after) separator) { ... }

The separatedBy and separatedByIndex might be combined into one, it's easy to ignore an argument, and cheap to increment an integer.

I'd expect the separator to not always have the same type as the elements being separated. (A List<TextWidgets> cannot be separated by SeparatorWidgets, one would have to first copy the text widgets into a List<Widget> and then separate them)

That makes a top-level function more useful (or stastic function or constructor, if we could add such using extensions):

  static Iterable<S> separatedBy<S>(Iterable<S> elements, S Function(int position)  separator) { ... }
  static Iterable<S> separatedByValue<S>(Iterable<S> elements, S  separator) { ... }
  static Iterable<S> separatedByElements<S, T extends S>(Iterable<T> elements, S Function(T before, T after)  separator) { ... }

Maybe we can make the separator optional, like:

  static Iterable<S> separatedOptionalBy<S>(Iterable<S> elements, S? Function(int position)  separator) { ... }
  static Iterable<S> separatedOptionalByElements<S, T extends S>(Iterable<T> elements, S? Function(T before, T after)  separator) { ... }

where returning null means no separator.
(Consider: separatedOptionalByElements(pathSegments, (before, after) => before.endsWith('/') || after.startsWith('/') ? null : '/') where we only insert a separator where it's needed. Maybe this is too specialized, and a join with conditional separator would be more relevant.)

@mosuem
Copy link
Member

mosuem commented Oct 21, 2024

Closing as the dart-lang/collection repository is merged into the dart-lang/core monorepo. Please re-open this PR there!

@mosuem mosuem closed this Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] add mapSeparated method on Iterable
4 participants