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

Using kts files instead of .gradle #807

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

Using kts files instead of .gradle #807

wants to merge 20 commits into from

Conversation

pfmaggi
Copy link
Collaborator

@pfmaggi pfmaggi commented Dec 9, 2019

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Moving to Kotlin DSL for the gradle build scripts.

💡 Motivation and Context

Having a single language both for the app and for its build script.

💚 How did you test it?

./gradlew check

📝 Checklist

  • I ran ./gradlew spotlessApply before submitting the PR
  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing

🔮 Next steps

Improve KTS design and build speed.

📸 Screenshots / GIFs

@keyboardsurfer keyboardsurfer mentioned this pull request Dec 18, 2019
8 tasks
@florina-muntenescu
Copy link
Collaborator

would you mind re-basing when you get the chance, please

@pfmaggi
Copy link
Collaborator Author

pfmaggi commented Dec 30, 2019

Rebase on master plus, a commit to add the main Glide artifact to kapt classpath. This is required having disabled compile avoidance for kapt in master:

kapt.include.compile.classpath=false

@florina-muntenescu florina-muntenescu changed the title Pfm/kts Using kts files instead of .gradle Jan 2, 2020
Copy link
Collaborator

@florina-muntenescu florina-muntenescu left a comment

Choose a reason for hiding this comment

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

Overall, for someone with no experience with kts, this looks good.
Maybe someone else, could take a look as well.


# Use kapt in parallel
kapt.use.worker.api=true
## Use kapt incremental
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this not needed anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Incremental annotation processing is enabled by default starting from version 1.3.50.

- Split the config object into three separate files based on content
- Cleanup main build.gradle.kts
- Incremental annotation processing is now enabled by default
- Too many kts in "build.gradle.kts.kts.kts".
Copy link
Collaborator

@keyboardsurfer keyboardsurfer left a comment

Choose a reason for hiding this comment

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

AFAICT depconstraints only hosts a build file. Does it need to be its own project for that?

app/build.gradle.kts Show resolved Hide resolved
buildSrc/src/main/java/Versions.kt Outdated Show resolved Hide resolved
depconstraints/build.gradle.kts Outdated Show resolved Hide resolved
depconstraints/build.gradle.kts Outdated Show resolved Hide resolved
app/build.gradle.kts Outdated Show resolved Hide resolved
buildSrc/src/main/java/Libs.kt Outdated Show resolved Hide resolved
buildSrc/src/main/java/Libs.kt Show resolved Hide resolved
id("maven-publish")
}

val appcompat = "1.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why arent' these constant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would prefer to not put them in an object: "const val are only allowed on top level or in objects

@Suppress("MayBeConstant") // Improve perf when changing values

object Libs {
val AX_ANNOTATION = "androidx.annotation:annotation:"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either const & SCREAMING_SNAKE_CASE or non const & pascalCase

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

const & screaming

gradle.properties Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@pfmaggi pfmaggi left a comment

Choose a reason for hiding this comment

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

This is a special components that needs to be consumed by the other modules:
https://docs.gradle.org/current/userguide/java_platform_plugin.html#sec:java_platform_consumption

e.g. it needs to be a project that can be imported:

dependencies {
    // get recommended versions from the platform project
    api(platform(project(":platform")))
    // no version required
    api("commons-httpclient:commons-httpclient")
}

@codingjeremy codingjeremy changed the base branch from master to main September 29, 2020 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants