-
Notifications
You must be signed in to change notification settings - Fork 11
Add Scala support #113
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 Scala support #113
Conversation
To me, I would like to have a refactoring at current stage. Because:
I'm sorry that this is a debt that I missed when implementing. About the refactoring, I'm thinking that
We can do the second refactoring first to unblock the scala support. The rough idea in my mind is:
WDYT? |
It sounds OK. I'm happy to code around your design, I'm just don't know whether that's the right direction as I haven't seen enough different language setups in Gradle to make a decision on splitting the design across languages. One big advantage here is that I doubt anyone will be using this project as a library (it doesn't make sense to me to do that) so I think you can change the design/API as and when you like without affecting downstream projects. Just some thoughts, not blockers...
|
Thank you @Arthurm1 for your input. Here is my plan: I'll first make sure those opened PR get merged recently. At the meantime, I'll find some time think thoroughly about how to do the refactoring. We can continue the discussion here. |
I'm not very familiar with Android, which kind of property of the source set is not aligned when it comes to Android projects? |
@Arthurm1 I now have a feeling that, maybe the BSP related types should be constructed at the plugin side, and the server is just simply get them from plugins and return them when different requests come. Currently the server needs to know the definition of the source set and parse them to build target, etc... This makes trouble as you've mentioned, the concept of source set does not exist for Android, not to say other languages. |
Maybe - I don't think that's a bad idea - I'm just not sure yet. Android does have the concept of sourcesets - it just has it's own hierarchy. What I do think is that this data might have to be cached. Have you tried the plugin on a big project? e.g. Gradle itself. It takes about a minute to respond to build/initialize (which I think should be instant and is slow because the source sets are queried in response to this request but not yet needed). Then, after a compile, the source sets are all queried again. It would be good to delay the building of the sourcesets until the client requests them and do they need to be built again after every compile? |
About the perf issue, let's discuss at #118.
Does that mean that for android project, we need a different way to fetch the information of the source set, but the properties of the source set are the same? |
I think so. |
@Arthurm1 I have got some ideas about the refactoring. I'll make a draft these days and let you review it. |
plugin/src/main/java/com/microsoft/java/bs/gradle/plugin/SourceSetsModelBuilder.java
Outdated
Show resolved
Hide resolved
plugin/src/main/java/com/microsoft/java/bs/gradle/plugin/SourceSetsModelBuilder.java
Outdated
Show resolved
Hide resolved
@Arthurm1, Sorry bro for the long time hanging. Would you like to continue the work based on the latest code base? |
@jdneo In the first commit, I've refactored for the new multi-language setup. The only thing of note is I've moved In the second commit, I've refactored the extension conversion. I've managed to make it so the conversion to a The only requirement after that is the casting of See what you think |
@jdneo If you're happy with this refactoring then I'll refactor the Kotlin PR |
model/src/main/java/com/microsoft/java/bs/gradle/model/JavaExtension.java
Outdated
Show resolved
Hide resolved
plugin/src/test/java/com/microsoft/java/bs/gradle/plugin/GradleBuildServerPluginTest.java
Outdated
Show resolved
Hide resolved
plugin/src/main/java/com/microsoft/java/bs/gradle/plugin/ScalaLanguageModelBuilder.java
Show resolved
Hide resolved
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.
Implements
buildTarget/scalacOptions
. Also returnsScalaBuildTarget
instead ofJvmBuildTarget
if Scala has been configured for that target and passes backscala
as a supported language.I haven't separated out the implementation per language. I think a refactor would be better done in a single PR (not one where a new language is added). I'm not sure it's currently worth refactoring. Maybe when more languages are added?
Creating the scala compiler args using
ZincScalaCompilerArgumentsGenerator
is a bit verbose because of the class it requires to extract the setup from. It only actually needs 2 methods to be implemented. Another option would be to copy the code directly fromZincScalaCompilerArgumentsGenerator
intoSourceSetsModelBuilder
. I thought this would be easier to maintain if the original is changed.