-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Support kotlin coroutines #1706
Conversation
This looks really nice. It makes total sense to make this available on the snyk is asking to "Upgrade org.jetbrains.kotlin:kotlin-stdlib to version 1.6.0 or higher" Sounds like a solid plan, once you are ready to move to a new module, fee free to ping me for any assistance needed |
6c88458
to
fc89552
Compare
@velo I pushed some changes
|
@velo I came up with a good idea. Looking at the Spring code a little more, it seems that I am handling it smarter than I thought. I'll look into it a bit more and update. |
I’m so happy someone picked this up. @velo could I have a day or two to review this as well? |
You absolutely can sir! But, until this is moved to a module, I wouldn't be comfortable to merge, as core needs to be lean and have no dependencies. |
Exactly why I wanted to review. Like minds |
I'm considering splitting modules using Optional Dependencies. What do you think about this approach? After a brief investigation, it seems that Spring Core also supports Kotlin using Optional Dependencies. I'm going to try implementing it today using optional dependencies, and investigate further if there are other approaches. (I live in the KST time zone (UTC+9), and I can write code after work.)
|
I don't think |
3e7499d
to
407ab47
Compare
@velo I pushed some changes to share idea.
|
* @author Sam Brannen | ||
* @since 1.1 | ||
*/ | ||
public abstract class ClassUtils { |
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.
This class (and others on the PR) look like copy and paste from somewhere.... please use the original library
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.
Assert.java / ClasUtils.java / KotlinDetector.java is from spring core project -> I will refactor when the implementation approach is agreed upon.
If the approach is agreed upon, Assert.java / ClasUtils.java used in KotlinDetector and KotlinDetector implementation will be refactored. These classes are from Spring Core. If I use the original library, I must use the Spring Core library.
How about reviewing where the feign code branches off using KotlinDetector for Kotlin support? I wrote the code only to demonstrate the approach.
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.
Assert.java -> Removed
ClasUtils.java / KotlinDetector.java -> Refactored
b228cff
to
9bc6d2b
Compare
Hi @wplong11 , so, as is, we can't merge, as we (Myself and @kdavisk6 ) won't approve any changes that introduce dependencies to feign core.... optional=>true is still a dependency, and that is not going to fly. Core needs ZERO dependencies. My plan is to refactor So, once you get your changes to a point that you are happy with, ping me and I will help you out. |
@velo Looking at the final result, Considerations
|
Not yet. But, I will get something done
Not really, it will bring in kotlin and whatever else it depends on. As is, you can't use feign if kotlin library is not included. |
@velo Looking at the Github Example project, it seems that feign-core is used with zero dependency. Am I misunderstanding something? |
either that dependency tree wrong, or a runtime exception will happen when performing an async operation |
I checked that Github Example in Feign project runs correctly without kotlin related modules. According to Maven document,
Kotlin related modules is not included in the classpath of Github Example. Is there a difference between not being included on the classpath and excluded from the dependency tree? +) Even though Spring Core uses kotlin related modules as optional dependency, It seems that all java only projects also run correctly without any kotlin related dependency |
@wplong11 I moved kotlin to a module, but couldn't get the tests working.... could you take a look, I must admit I don't know where to start |
} | ||
|
||
@Test | ||
fun shouldRun4(): Unit = runBlocking { |
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 we get meaningful names for tests?
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.
Corrected. Please provide feedback on the naming convention.
Thanks for the additional work. I've been too busy lately. I'll be able to start working again in two weeks. I'll finish this PR soon. |
Awesome, and thanks for this massive contribution. I have o my mind now, that I need to find a way to have simpler |
- Remove KotlinDetector.java - Add Method.isSuspend extension function
- Copy of GithubExample
|
Revert the public access modifier changed in PR OpenFeign#1706
Revert the public access modifier changed in PR OpenFeign#1706
Revert the public access modifier changed in PR #1706 Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>
Resolves: #1565
!! This is working PoC
Inspired by PlaytikaOSS/feign-reactive#486
TODO