Skip to content
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 DSL template #23

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add Kotlin DSL template #23

wants to merge 12 commits into from

Conversation

KosmX
Copy link

@KosmX KosmX commented Jan 4, 2023

No description provided.

@modmuss50 modmuss50 self-requested a review January 4, 2023 15:54
@modmuss50 modmuss50 added the enhancement New feature or request label Jan 4, 2023
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a great start, a few small changes, we should make sure that this is as close as possible to the groovy build script. Please let me know if you disagree with anything, if needed we can always change the groovy build script to match.

@KosmX KosmX requested a review from modmuss50 January 4, 2023 18:14
@@ -0,0 +1,113 @@
plugins {
id("fabric-loom") version "1.0-SNAPSHOT"
id("maven-publish")<% if (it.kotlin) { %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
id("maven-publish")<% if (it.kotlin) { %>
`maven-publish`<% if (it.kotlin) { %>

The standard Gradle plugins have extension properties that avoid the use of id. See Gradle's own example:

Copy link
Author

@KosmX KosmX Jan 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I personally like id("maven-publish") more. There are no ` characters and more consistent.
Maybe vote?

options.release.set(java.targetCompatibility.majorVersion.toInt())
}
<% if (it.kotlin) { %>
withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An import is better than the fqn imo

}

withType<JavaCompile> {
options.release.set(java.targetCompatibility.majorVersion.toInt())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be <%= it.java.release %> like in the groovy template?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't create more constants, This PR is about Kotlin DSL, but I think, it should be changed in groovy to reuse the targetCompatibility constant

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the best solution would be to use top-level constants in both Kotlin and Groovy buildscripts (val/def respectively) for these constants.

Since "this PR is about Kotlin DSL", shouldn't it follow the existing pattern?

}
<% if (it.kotlin) { %>
withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile> {
kotlinOptions.jvmTarget = java.targetCompatibility.toString()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and shouldn't this be <%= it.java.kotlinRelease %>?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again... no magic constants if possible.


jar {
from("LICENSE") {
rename { "${it}_${base.archivesName}" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rename { "${it}_${base.archivesName}" }
rename { "${it}_${base.archivesName.get()}" }

archivesName is a Property<String>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. but it worked without get()

@modmuss50 modmuss50 requested a review from Juuxel May 29, 2023 12:26
scripts/src/lib/Template.svelte Outdated Show resolved Hide resolved
}

withType<JavaCompile> {
options.release.set(java.targetCompatibility.majorVersion.toInt())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the best solution would be to use top-level constants in both Kotlin and Groovy buildscripts (val/def respectively) for these constants.

Since "this PR is about Kotlin DSL", shouldn't it follow the existing pattern?

withType<JavaCompile> {
options.release.set(java.targetCompatibility.majorVersion.toInt())
}
java {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
java {
java {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants