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.

Sign up for GitHub

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

Jump to bottom

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
modmuss50
modmuss50 requested changes Jan 4, 2023
View reviewed changes
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.

scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated Show resolved Hide resolved
scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated Show resolved Hide resolved
scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Show resolved Hide resolved
scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated Show resolved Hide resolved
scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated Show resolved Hide resolved
scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated Show resolved Hide resolved
@KosmX KosmX requested a review from modmuss50 January 4, 2023 18:14
Juuxel
Juuxel suggested changes Jan 4, 2023
View reviewed changes
scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta
@@ -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 •
edited
Loading

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?

scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated Show resolved Hide resolved
scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated Show resolved Hide resolved
scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated
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

scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated Show resolved Hide resolved
scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated Show resolved Hide resolved
scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta
}

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?

scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated
}
<% 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.

scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta Outdated

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
Juuxel
Juuxel suggested changes May 29, 2023
View reviewed changes
scripts/src/lib/Template.svelte Outdated Show resolved Hide resolved
scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta
}

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?

scripts/src/lib/template/templates/gradle/kotlin/build.gradle.kts.eta
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
Reviewers

@Juuxel Juuxel Juuxel requested changes

@modmuss50 modmuss50 modmuss50 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@KosmX @modmuss50 @Juuxel