Closed ilya-g closed 7 years ago
Do we need peek
for Iterables at all?
Some counter-arguments:
apply { forEach { ... } }
this
Iterable
usually copy the source collection, it may be hard to tell what would be the behavior of peek
Queue.peek()
.I like the name onEach
as a complement to forEach
since it further implies that this is purely used as a side-effect operation in the stream/chain.
Should
peek
delegate tomap
for Sequences?
We plan to make some optimizations of map
operation for sequences, it would be beneficial if peek
could exploit those optimizations without having to reimplement them.
To implement peek
with map
we have to wrap the operation in the following lambda:
{ operation(it); it }
We could either inline the operation in that lambda or not. The performance consequences of that options should be studied for both cases: when the operation is specified as a lambda itself peek { println(it) }
, or passed as a functional type peek(operation)
.
can be expressed with
apply { forEach { ... } }
Another con: It creates another nested layer of curly braces and thus more indentation in many cases.
intermediate operations on
Iterable
usually copy the source collection, it may be hard to tell what would be the behavior ofpeek
I have to agree here - the behavior definitely differs from Sequence.peek
and could cause misunderstanding.
sequenceOf(1, 2).peek { println(it * 2) }.forEach { println(it) }
would yield
2
1
4
2
while
listOf(1, 2).peek { println(it * 2) }.forEach { println(it) }
would yield
2
4
1
2
can be confused with
Queue.peek()
That would be sorted out by renaming peek
to onEach
- perhaps only Iterable.peek -> Iterable.onEach
, but not Sequence.peek -> Sequence.onEach
, to reflect the differences in behavior. A downside would be decreased discoverability.
The performance consequences of that options should be studied for both cases
Unfortunately I don't have much experience with this, especially since there may be differences between various VM implementations like ART, Dalvik and the JVM due to the JITs.
That would be sorted out by renaming peek to onEach - perhaps only Iterable.peek -> Iterable.onEach, but not Sequence.peek -> Sequence.onEach, to reflect the differences in behavior. A downside would be decreased discoverability.
The difference in evaluation order is not an issue, the same difference is observed if you change peek
to map
for example.
After some contemplation I think onEach
is a good name both for Iterable
and Sequence
.
I believe it's the time to implement a prototype. You can either craft it as a pull request to the kotlin standard library if you're familiar with stdlib code generator or write manually in any other repository.
Some implementation notes:
Iterable<T>.onEach
returns the receiver itself, so it would be good to have the return type preserving the type of the receiver.onEach
could be provided for other types of receivers that already have forEach
.If I understood the DSL correctly, something like this should work. Ant kept crashing for me and I'm getting Could not find artifact org.jetbrains.kotlin:kotlin-maven-plugin:pom:1.1-SNAPSHOT
when executing the maven goal compile exec:java
on the generator, so unfortunately I wasn't able to test it.
peek
is a really confusing name imo, because peek
as "look at the next element, but don't call next" also makes sense for iterators. Rust uses the name inspect
for this function, I think it is a good choice.
@cbruegg not quite right, but ok for starting point. I've elaborated those templates a bit, it required to improve the DSL to support such non-trivial constraints on generic receiver.
The result is available in rr/stdlib/oneach branch. Could you check it out and try it with some tests?
@ilya-g Sorry for the late response and thanks for fixing the template. I've had a look at the generated code and tried it out with some tests and everything seems to work as expected. I really like how we were able to implement this using map
and apply
, which makes the generated code easily verifiable for correctness.
Personally, I think this looks pretty much complete.
Since we'd like to avoid inflating method count in stdlib, we're going not to provide onEach
for arrays. Otherwise, all looks good and we'll merge rr/stdlib/oneach branch soon.
Great, thank you for having worked on this! It's always great to see that JetBrains is really listening to feedback.
What about onEachIndexed
?
Wanted to reask, what is about onEachIndexed?
I think that onEachIndexed
can be useful.
Could you provide some motivating examples for this function?
Discussions about the
onEach
operation for sequences and iterables will be held here.