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

Use versions catalog for convention plugins #1

Merged

Conversation

piloudu
Copy link
Contributor

@piloudu piloudu commented Jul 23, 2023

Description

This technical improvement PR aims at providing type-safe plugin references by registering the convention plugins in the libs.versions.catalog file

Some advantages:

  • Centralise logic in the versions catalog
  • Provide type-safe plugin references

@piloudu piloudu requested a review from Morfly as a code owner July 23, 2023 14:34
@piloudu piloudu force-pushed the TechnicalImprovement-UseAliasForGradlePlugins branch from 105910b to eef9c89 Compare July 23, 2023 14:38
@Morfly
Copy link
Contributor

Morfly commented Jul 27, 2023

Thank you for your contribution @piloudu!
Would you mind squashing the commits and adding a message in a format
feat: <preferred name>, to avoid the lint warning, please?

We can make kotlin("jvm") and id("kotlin-parcelize") plugins as aliases too.

@piloudu piloudu force-pushed the TechnicalImprovement-UseAliasForGradlePlugins branch from eef9c89 to ab50724 Compare July 28, 2023 05:57
@piloudu
Copy link
Contributor Author

piloudu commented Jul 28, 2023

Thank you for your contribution @piloudu! Would you mind squashing the commits and adding a message in a format feat: <preferred name>, to avoid the lint warning, please?

Hi @Morfly! Sure, I've just made those changes. Could you check again if everything looks good for you?

We can make kotlin("jvm") and id("kotlin-parcelize") plugins as aliases too.

I've applied the same approach to the kotlin("jvm"), id("kotlin-parcelize") and kotlin(kapt) plugins. For these cases an alias can't be used but we can still use id(libs.plugins.<pluginNameInCatalog>.get().pluginId). The reason is:

// Using alias as follows...
plugins {
    alias(libs.plugins.sample)
}

// ... is the same as applying the plugin with an specific version
plugins {
    id(libs.plugins.sample.get().pluginId) version libs.plugins.sample.get().version apply true
}

Taking this into consideration, any id("<pluginId>") that doesn't specify the plugin version must be converted into id(libs.plugins.<pluginNameInCatalog>.get().pluginId)

gradle/libs.versions.toml Outdated Show resolved Hide resolved
@Morfly
Copy link
Contributor

Morfly commented Jul 28, 2023

@piloudu thank you for the update.

We can still use these plugins as alias
In order to do it, we need to explicitly apply them in the root build.gradle.kts file similarly to other plugins.
Also, as for kotlin-parcelize please read my comment in the code, it could be also applied as an alias.

gradle/libs.versions.toml Outdated Show resolved Hide resolved
@piloudu piloudu force-pushed the TechnicalImprovement-UseAliasForGradlePlugins branch from fdd6998 to 66c6245 Compare July 28, 2023 06:33
@piloudu piloudu force-pushed the TechnicalImprovement-UseAliasForGradlePlugins branch from 66c6245 to cfa956d Compare July 28, 2023 06:51
@piloudu
Copy link
Contributor Author

piloudu commented Jul 28, 2023

@piloudu thank you for the update.

We can still use these plugins as alias In order to do it, we need to explicitly apply them in the root build.gradle.kts file similarly to other plugins. Also, as for kotlin-parcelize please read my comment in the code, it could be also applied as an alias.

That's a great point! I didn't know that id("pluginId") declarations could be translated into alias ones by declaring the plugin in the root build.gradle.kts file.

I've performed those changes and I'm up to new suggestions if needed

gradle/libs.versions.toml Outdated Show resolved Hide resolved
@piloudu piloudu force-pushed the TechnicalImprovement-UseAliasForGradlePlugins branch from cfa956d to 0832af5 Compare July 28, 2023 06:57
Copy link
Contributor

@Morfly Morfly left a comment

Choose a reason for hiding this comment

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

Thank you for making the updates to the PR @piloudu. all looks good to me!

@piloudu
Copy link
Contributor Author

piloudu commented Jul 28, 2023

Thank you for making the updates to the PR @piloudu. all looks good to me!

Great! Thanks @Morfly for taking the time to review this PR and kudos on this great library 👏

@Morfly Morfly merged commit 0ca20df into open-turo:main Jul 28, 2023
2 checks passed
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.

None yet

2 participants