-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Create Kotlin extension #1059
Create Kotlin extension #1059
Conversation
sleep(); | ||
filter(source, ImmutableMap.of(uuid, "carambar")); | ||
|
||
// Wait until we get "uuid" |
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 should be "carambar", not "uuid" :)
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.
True, I just copied it from one the existing tests. I'll fix them
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.
Fixed
Nice! I think we probably want some kind of |
@stuartwdouglas If you have an SPI in mind, then I'd gladly refactor the PR to use it (while also moving it into a new module). Whatever you think is appropriate, I'll gladly roll with! |
.filter(p -> wasRecentlyModified(p)) | ||
.map(Path::toFile) | ||
//Needing a concurrent Set, not many standard options: | ||
.collect(Collectors.toCollection(ConcurrentSkipListSet::new)); | ||
.collect(Collectors.toSet()); |
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 we still need a concurrent set, or? Collectors.toSet()
is using HashSet
...
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 is only used to test and log stuff. The actual collection that the compiler uses is created a few lines down
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.
Well, you still need some sort of synchronization because it's a parallel stream. And vice versa - you probably don't need the ConcurrentSkipListSet
below...
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.
A damn... You are absolutely right! I didn't see the parallel stream...
Thanks for spotting it! I'll fix it ASAP
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.
Fixed
I think it would be smart to have a kotlin guide for the first public release. I did try to use kotlin with quarkus last week and faollowed the kotlin guide and it just worked (basics of course). The only missing piece was the dev mode. So I'd merge this PR and create the guide and possibly create some extension/improve later? |
If we get this in, then I will also create a guide for it. Then once quarkus is released I can proceed to create an kotlin compiler plugin to automatically "open" classes with certain annotations (just like the kotlin-spring plugin). |
Did you try a "mixed change"? I mean, reloading after a change both in a Java source and Kotlin source, where one depends on the other. My understanding is that mixed projects are relatively common in the Kotlin world. |
I have not and my guess is that it probably won't work, but I am fairly certain that we could get it to (I vaguely remember seeing some flags about compiling Java). |
If we had a I suggest that we merge this (if it's up to the project standards that is) and then follow up with a full blown Kotlin extension with a first version of the compiler SPI if time permits. |
According to this it's possible, but I'm not sure it's something we want to try to support |
I (seemingly) have some extra cycles so I will look into breaking this work out into a Kotlin extension. |
a690087
to
0fbfeeb
Compare
@geoand btw, we will need some sort of quickstart for this feature as we need to shed some light on it. |
@gsmet Yes of course. I'm working on breaking things out into an extension now. Once that is done, I will also add a quick-start and a guide |
I know it's possible, as it's a big part of their gradual Java -> Kotlin migration story. I think you already invoke the Kotlin compiler before the Java compiler, so it should be just a matter of passing some arguments to it, I think? :-) |
I don't really to know to be honest :). Having everything work OOTB for mixed projects would be awesome, but I don't think I have enough time to get it done properly at the moment. |
The PR now introduces the ability to generate a properly configured Maven+Kotlin project when the Maven So I would say that it should be complete for the first release. Please give it a try and let me know what you think |
I need to see why the tests are failing. I'll do that in a few hours |
Tests should pass now |
Rebased onto latest bom/build-parent changes |
Are we planning to merge this before the first public release? Just asking because in that case I need to start writing a guide as well. |
@geoand could you squash things a bit? I think it's something we would want but I can't assure you we will have the cycles to review it. Can you start working on the guide anyway? That won't be time wasted as we will integrate this at some point. |
Adds support for a hot reload in Kotlin and also gives the ability to create Kotlin source code from the Maven plugin
@gsmet I squashed to a single commit. I'll start writing on Monday and if we want to merge this then I'll get it done ASAP |
The basic functionality of changing a Kotlin source file and having the
quarkus:dev
recompile seems to work fine.There are of course a few things I don't really like:
In order for the devmode to work with Kotlin, the user needs to addThis is somewhat analogous with what users have to do with
spring-boot-devtools
.I am guessing we could alternatively download the that artifact and make it available on the user's classpath when we detect that we are working on a Kotlin app?
Although the diagnostics work, they could obviously use some polish in how they are displayed
The implementation could use a lot more polish to abstract away which compiler is to be used. I just went for the fastest thing I could do to get up to speed.UPDATE
See: #1059 (comment) for the latest on what the PR provides