Small fixes and more tests for jdk Converters #7970
Conversation
|
Also has a few fixes
Review by @Ichoran |
|
I'll try to get to this tomorrow--I probably won't have time today. Thanks for grabbing my fixes. |
There was a problem hiding this comment.
Since it's mostly tests I didn't read through exceedingly carefully; everything looks good that I saw. The one thing I caught is that the newest collections don't seem to be part of the tests: CollisionProofHashMap, VectorMap, and TreeSeqMap. But collection-laws tests those three and covers many of the cases here, so I don't think it's critical.
(I'm a little wary of the mima exceptions because they might mask actual incompatibilities later if we need to change something, but I guess we're starting bincompat with RC1?)
There are a few open issues
- ChampStepper doesn't return all elements (Rex is working on a fix)
- TableStepper.trySplit has a bug, it was changed to always return
`null` in a previous commit (Rex is also fixing this)
- The `ORDERED` flag is often not correct, in particular when creating
a Stepper of a Set, and there's no specific implementation. Then
the implementation falls back to IterableOnce, and IteratorStepper
has the `ORDERED` flag
All `trySplit` instances now pass in collection-laws. The Champ split is not as even as it could be; better would be to extract the sizes from the nodes and use that to wisely pick the partition points. However, it's not binary compatible.
added those
We'll reset MiMa to 0 on the 2.13.0 release, so we can still break bin compat for now. |
|
Changes look good. I was uncertain at the time whether to use |
|
Thanks! |
|
@adriaanm this is ready. |
|
I added a few more commits. The last one is to be discussed. I changed the references to primitives to the boxed types in the explicit conversion methods. For example - def toJavaOptionalDouble(o: Option[Double]): OptionalDouble = o match {
+ def toJavaOptionalDouble(o: Option[jl.Double]): OptionalDouble = o match {The reason is that Scala primitives in generic signatures erase to The problem is that using the explicit conversion methods in Scala is now pretty annoying, which might be very confusing to poeple. As an alternative, we could duplicate the |
Do you know why? |
|
@adriaanm I removed the last commit (to use boxed types instead of scala primitives) here, will split it off to a separte PR. So this one is ready. |
|
@viktorklang I copied the |
|
@lrytz I think I fixed the test here: viktorklang@c141ee3 The problem is that the callback is both executed synchronously and completed synchronously, which means that the calling thread is waiting for the latch to trigger, so it cannot trigger the latch. I.e. the test is wrongly written. I "fixed" the test by changing the order of the completion and the countdown, but an equal solution would be to switch to *Async versions of the methods (sadly there are no such options for all of the methods tested. I also fixed a test which demands that obtrude throws UnsupportedOperationException. Let me know if you want to discuss this further! |
|
One item of note that I discovered while doing this is that there does not seem to be a cohesive story about whether to call converters toX or asX |
|
It is supposed to be the case that Maybe this implementation detail is not sufficiently important to warrant a seemingly random selection of |
|
I followed that rule consistently in the |
|
@viktorklang will look tomorrow at your proposed changes, thanks. I wonder why the test passes on java8-compat (with 2.13.0-RC1, scala/scala-java8-compat#139). |
|
@lrytz Hmmm, I do not know why it succeeds on the other version. Let me know if you have any follow-up questions tomorrow. |
|
Oh, I see that I skipped a lot of method overrides when porting FuturesConvertersImpl.CF over to the Scala repository. Adding them fixes the test. So this is totally my fault, sorry... |
|
@lrytz Np! I'm glad the test caught it though! :-) |
Not sure why I missed those initially...
|
|
||
| override def thenCompose[U](fn: JFunction[_ >: T, _ <: CompletionStage[U]]): CompletableFuture[U] = thenComposeAsync(fn) | ||
|
|
||
| override def whenComplete(fn: BiConsumer[_ >: T, _ >: Throwable]): CompletableFuture[T] = whenCompleteAsync(fn) |
There was a problem hiding this comment.
@lrytz @SethTisue @viktorklang Why all these methods are delegated to the async ones?
There was a problem hiding this comment.
There is a comment above:
// Ensure that completions of this future cannot hold the Scala Future's completer hostage
It seems these overrides were always there scala/scala-java8-compat@b3fca14
Haven't thought about it in detail, but maybe there are separate thread pools for Scala and Java futures, and it's to avoid running on the wrong one?
There was a problem hiding this comment.
Amazingly, I could scroll back three weeks in Discord and find the link I posted.
Include tests from java8-compat, and add some new ones.
Fixes scala/bug#11468