-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Consolidate native dependencies versions #20742
Conversation
apply plugin: 'maven' | ||
|
||
apply plugin: 'de.undercouch.download' | ||
plugins { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think old-fashioned is good and more readable.
But this is recommended way and Kotlin DSL compatible. IMHO, Gradle is
recommending to have configuration blocks to make more readable and
performant.
|
Yes, but most project like butter-knife and rxjava use the old-fashioned way. I think kotlin dsl still has a long way to go. Even sqldelight and okio which written in kotlin use the old-fasioned way too https://github.com/square/sqldelight/blob/master/sqldelight-core/build.gradle. |
Plugins block or configuration blocks in general are recommended way, and
it's more readable. I see no reason to stick with old syntax.
FYI, Kotlin DSL is now 1.0RC3 and will be stable with Gradle 4.10 very
soon. It has superior IDE support and more performant compared to Groovy
DSL.
|
I think the old way is more readable. And adopt to new kotlin dsl we can wait for google officially support it. No need to rush to new syntax since few people using it. |
Instead of following "other peoples recommendations", does using |
@hramos please review and merge. plugins block is recommended way to configure Gradle plugins and I found it's used in fresco. |
@hramos please review and merge. I think that we can optimize CI cache and save some time for developers once merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@dulmandakh merged commit ba608a2 into Once this commit is added to a release, you will see the corresponding version tag below the description at ba608a2. If the commit has a single |
Summary: This PR tries to consolidate Android native dependencies versions to make it less error prone to version bumps. Pull Request resolved: facebook#20742 Differential Revision: D9818155 Pulled By: hramos fbshipit-source-id: 9bf631640910edad5731014f4e23dbca45af2b59
Summary: This PR tries to consolidate Android native dependencies versions to make it less error prone to version bumps. Pull Request resolved: #20742 Differential Revision: D9818155 Pulled By: hramos fbshipit-source-id: 9bf631640910edad5731014f4e23dbca45af2b59
Summary: This PR tries to consolidate Android native dependencies versions to make it less error prone to version bumps. Pull Request resolved: facebook#20742 Differential Revision: D9818155 Pulled By: hramos fbshipit-source-id: 9bf631640910edad5731014f4e23dbca45af2b59
This PR tries to consolidate Android native dependencies versions to make it less error prone to version bumps.
Test Plan:
CI is green