Add support for CompletableFuture for method return types#638
Conversation
|
There are few things that will help make this PR ready for review. Please see CONTRIBUTING for more information. Also, this does appear to push Java version up. In the past we have tried not to do this, for various reasons. Take a look at HACKING for more tips. If this feature gets enough support, it may be better of in a new |
There was a problem hiding this comment.
I've spent a little more time researching this. Hystrix does not support CompleteableFuture internally. Your implementation uses a common workaround, by turning the future into an Observable. It is my opinion that until Hystrix supports this directly, we consider this a JDK8 only update and move it to the jdk8 module, separating it from Hystrix entirely.
| @@ -0,0 +1,14 @@ | |||
| # top-most EditorConfig file | |||
There was a problem hiding this comment.
I'm not sure we should be including editor configuration files this way.
|
|
||
| import java.util.concurrent.CompletableFuture; | ||
|
|
||
| @IgnoreJRERequirement |
There was a problem hiding this comment.
Personally, I don't like this kind of thing. If this code requires JDK8, then it should be in the jdk8 module.
There was a problem hiding this comment.
Is there any strong reason for our current java 6 constraint?
There was a problem hiding this comment.
It is my understanding, from reading previous comments and documentation from @adriancole, it is the opinion of this project that core feign should remain JDK 6+ and that all JDK 8 features are in the feign-java8 module. This PR will require core feign to move to JDK 8, which has been called out in other PR and issues before as something we should try to avoid.
Personally, I do not have a strong opinion on this. My questions and thoughts are an attempt to stay consistent. I am open to having a discussion with the other contributors to see if we should revisit this decision.
There was a problem hiding this comment.
I change to jdk 8 baseline should require a major revision (ie 10.x)
There was a problem hiding this comment.
change to jdk 8 baseline should require a major revision (ie 10.x)
LGTM =)
|
@wjam Master is now Java 8, if you still want this PR to move forward, can you take a look and see if you can apply it to the new baseline? |
Implements support for `CompletableFuture` on method return types by converting through RxJava `Observable`
kdavisk6
added
waiting for feedback
|
@wjam This PR limits support to |
kdavisk6
added
the
waiting for feedback
Implements support for `CompletableFuture` on method return types by converting through RxJava `Observable`
Implements support for `CompletableFuture` on method return types by converting through RxJava `Observable`
4 participants
Implements support for
CompletableFutureon method return types by converting through RxJavaObservable