-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add IterableOps#tapEach method #7124
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -249,4 +249,10 @@ trait StrictOptimizedIterableOps[+A, +CC[_], +C] | |
| (l.result(), r.result()) | ||
| } | ||
|
|
||
| // Optimization avoids creation of second collection | ||
| override def tapEach[U](f: A => U): C = { | ||
| foreach(f) | ||
| coll | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can't return
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, It seems that this solution does solve it, as long as you also enforce the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exceptions used to be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @szeiger I can't think of any other improvements or simplifications that could be made if it had |
||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could I get others' opinions on whether or not this should be implemented as a call to
mapinstead offromSpecific(new View.Map)? I think that the former simplifies/reduces the work for those implementing collections which need to overridemap, but I am curious to hear others' thoughts.I don't think the decision should hold up the PR - it's an easy change to make in its own PR if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using
mapis better, indeed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, calling
map { a => f(a); a }will only return aCC[A], rather than the more specificC.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is interesting and unfortunate. I'm torn on whether or not it would still be better to use
mapand cast the result. I'm leaning towards not.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked @szeiger to look at this and explain it to me.
Josh's point is that we can't typecast our way out of this.
CCandC[A]can be different collection types entirely:(in an earlier edit, I'd included a
Stringexample where I "upcast" it toIterable[Char], but that's not really the same, because it's an implicit conversion, not an upcast)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SethTisue Wow! Very sneaky! Some downsides (which may have workarounds):
Iterable. So the consequences of this are that, perhaps non-strict Seq's like SeqView and LazyList will have to then go through each element in order to fulfill a call toapply(100), whereas before thefiltercall, they may have been able to independently evaluate/access element100immediately.IndexedSeqView#filtercannot return anIndexedSeqViewThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add on one more point,
filteralso destroys independent laziness of elements (currently only a thing forLazyList, but could potentially be a thing for some lazyIndexedSeqin the future).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshlemer okay, sounds like it’s better as-is then
@NthPortal I don’t follow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SethTisue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I retract all complaints, and I'm happy with
fromSpecific(new View.Map(...))