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

Hotfix broken build with a test #1545

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

Conversation

Jaehwa-Noh
Copy link
Contributor

What I have done and why

Fix #1544

Change-Id: I07f355351e6d6afba09e8d4af512a8a087121e93
@Jaehwa-Noh Jaehwa-Noh changed the title Hot fix broken build Hotfix broken build Jul 12, 2024
Copy link

@machado001 machado001 left a comment

Choose a reason for hiding this comment

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

Imho we could add a new line to libs.versions.toml to maintain project consistence.

in libs.version.toml:

[libraries]
//another libs...
kotlin-test = { group = "org.jetbrains.kotlin", name = "kotlin-test", version.ref = "kotlin" }

then:

dependencies {
    ...
    testImplementation(libs.kotlin.test)
    ...
 
}

What do you think?

@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Jul 14, 2024

@machado001
Thanks for your comment.

From this documentation.
https://kotlinlang.org/docs/gradle-configure-project.html#set-dependencies-on-test-libraries

You can use shorthand for a dependency on a Kotlin module, for example, kotlin("test") for "org.jetbrains.kotlin:kotlin-test".

We use Kotlin DSL and it gives us kotlin shorthand kotlin("test")

kotlin("test") is more shorten and clear than version catalog, and it reduces that what we have to manage a dependency as we delegate it to kotlin.

I found same thing on Groovy also can use shorthand.

kotlin {
    sourceSets {
        commonTest {
            dependencies {
                implementation kotlin("test") // This brings all the platform dependencies automatically
            }
        }
    }
}

@machado001
Copy link

Hi @Jaehwa-Noh, thank you for your reply.

I agree that using kotlin("test") is clean enough. Just to provide another perspective, I suggested adding the library to the version catalog to maintain project consistency and have a centralized place to manage and document dependencies.

New developers can start contributing to the project by looking at the version catalog file to get a general overview of the libraries and plugins used in the project.

I checked another build.gradle.kts file, particularly from the :lint module, and it looks like kotlin("test") is already used there. This could become a recurrent dependency that isn't declared in the version catalog, requiring developers to check each module individually to discover this.

Thus, another advantage is that, from the version catalog, developers can simply Ctrl + click (or Ctrl + B) to check which modules use the dependency.

@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Jul 14, 2024

@machado001 Yes, you have the reason. Simple way is you can found all usage in search all files Ctrl + Shift + F (Find all files).
On this topic, I can't determine which way is better on my own.
Let us hear the other developers' and maintainers' opinions.

@trietbui85
Copy link

I would prefer the explicit declaration of kotlin-test dependency in libs.versionss.toml.

Change-Id: If411597e83749126093fed5eb2e0c28ed60f9cb8
@Jaehwa-Noh Jaehwa-Noh changed the title Hotfix broken build Hotfix broken build with a test Sep 14, 2024
Change-Id: Ie56a69d242aef23171fdef9f39676228026167ec
@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Sep 14, 2024

Following #1560, not only fixed #1544 but also replaced kotlin("test") to version catalog kotlin.test for consistent.

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

Successfully merging this pull request may close these issues.

[Bug]: Build Broken
3 participants