-
-
Notifications
You must be signed in to change notification settings - Fork 358
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
Add Kotlin integration #3514
Add Kotlin integration #3514
Conversation
a983d13
to
da99464
Compare
I think not supporting kotlin-reflect is fine for this initial PR for Part 1 of the bounty. Part 2 of the bounty covers a lot more scenarios, so we can look at figuring out supporting kotlin-reflect and other fanciness (compiler plugins?) then, as the counterpart to |
… for kotlin modules
@lihaoyi Code-wise this PR is done, I think. I addressed your suggestions. The only remaining thing is to add Kotlin samples to the generated docs. |
The code looks good I think. The example tests just need to be tweaked I think to match the stdout format of the kotest library. You can run them locally via |
Don't forget to address copyright requirements. |
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'd expect some refactorings, since the original codebase was cross-built against multiple Mill versions. Some compromises had to be made to accomplish that, which are no longer necessary here. E.g. the KotlinModulePlatform
is superfluous and should be merged with KotlinModule
to keep things simple and understandable.
Beside the requirements of the original Apache License 2.0, I'd expect at least some traces of history in the documentation and/or Scaladocs. Not doing so not only pisses the original contributors of the code off (which is mostly me), but it also doesn't guide the user and developer in understanding the code and which relationship it has to the pre-existing mill-kotlin
project. I don't think, it's acceptable for an Open Source Software project, to dishonor others people work.
@lefou Yes, sure, it was on my to-do list before marking PR as final, after code changes are complete. I added the copyright now in 85c1f30. However, since https://github.com/lefou/mill-kotlin has no explicit copyright statement neither in the source files, nor in the license file, I had to come up with the following (since I traced back the first commit to 2020 and there are 2 contributors):
Feel free to suggest another one if needed. Regarding the refactoring: I merged |
@lihaoyi I fixed Kotlin tests, but there is one job ( |
What I actually meant to request is to respect the original license, which is Apache License 2. Although it allows you to do almost everything, it also demands to point it out and to include the license text somewhere in the project, if you use or derive the work. The fact that the actual code you included is from me is rather secondary here. My point is about free and open source work in general. It's only free if you comply to the license, whether you like it or not. And since this is missing in this pull request, I had rather not accepted this PR as-is. |
This PR solves Part 1 from #3451:
I still need to finalize it (especially automated test for example
4-builtin-commands
fails right now), but most of the work is done.Things I noticed in the original implementation (https://github.com/lefou/mill-kotlin):
ivyDeps
, meaning that if the user ofKotlinModule
wants to add its own deps and forget to callsuper.ivyDeps() ++ <their deps>
and just doesdef ivyDeps = <their deps>
, then no stdlib will be in the classpath during the compilation and it will fail with cryptic error. Such approach is thus error-prone.-nostdlib
flag is used for compilation)@lihaoyi Feel free to give your feedback while this is in a Draft state if you want.