Join the cool kids club (aka migrate to Gradle Kotlin DSL) #1321

SUPERCILEX · 2018-05-28T21:10:37Z

Surprise, I found another way to use Kotlin in FUI! 🎉😁

Signed-off-by: Alex Saveau <[email protected]>

# Conflicts: # auth/build.gradle # constants.gradle # library/quality/quality.gradle

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2018-05-29T18:20:02Z

@SUPERCILEX this is pretty cool! And does it fix the lint/aapt2 issue as well? If so I think we should release this as 4.0.1 which will also give me a chance to make sure that the full release process works with the refactored gradle files.

samtstern

Mostly small comments

samtstern · 2018-05-29T18:25:35Z

app/build.gradle.kts

@@ -0,0 +1,59 @@
+android {


Let's add a big comment on this file saying something like "hey we R cool kidz so we use the Kotlin DSL. If you're writing an app that uses FirebaseUI check out README.md for better examples" or something like that (please not that verbatim)

Haha, I'm definitely going to quote you on that one! 😁 Side note: do you think we should rename :app to :sample?

I've gone ahead with that change though it's in one commit we can easily kill if you disagree.

samtstern · 2018-05-29T18:27:57Z

auth/README.md

@@ -135,7 +135,7 @@ Twitter app as reported by the [Twitter application manager](https://apps.twitte
 In addition, you must enable the "Request email addresses from users" permission
 in the "Permissions" tab of your Twitter app.

-In order to resolve the Twitter SDK, add the following repository to your `build.gradle`:
+In order to resolve the Twitter SDK, add the following repository to your `build.gradle(.kts)`:


I think we should use the longhand "build.gradle or build.gradle.kts" everywhere to minimize confusion.

I've decided to simply remove those references since our samples won't compile in gradle.kts anyway. 🤷‍♂️

samtstern · 2018-05-29T18:28:18Z

auth/build.gradle.kts

+
+    lintOptions {
+        disable("UnusedQuantity")
+        disable("UnknownNullness") // Too much to deal with right now


Fine with the disabling but can you explain what's going on?

Yeah, I realize the nullability changes account for a lot more thrashing than originally anticipated. I've rebased those out and will make another PR with a better explanation. (As a short explanation, Tor is continuing his eternal quest to keep us on our toes with new lint checks. 😆)

samtstern · 2018-05-29T18:29:58Z

build.gradle.kts

+val Project.reportsDir get() = "$buildDir/reports"
+
+fun Project.configureAndroid() {
+    apply(plugin = "com.android.${if (name == "app" || name == "proguard-tests") {


This is over my personal fanciness limit :-)

How about:

if (name == "app" || name == "proguard-tests") { // apply app plugin } else { // apply library plugin

Haha, np. 😁😊

samtstern · 2018-05-29T18:31:14Z

build.gradle.kts

+}
+
+fun Project.setupPublishing(isLibrary: Boolean) {
+    val publicationName = "${if (isLibrary) "monolith" else name}Library"


I'd prefer not to have expressions in strings. How about:

val publicationName = if (isLibrary) "monolithLibrary" else "${name}Library"

samtstern · 2018-05-29T18:34:29Z

buildSrc/src/main/kotlin/Config.kt

+            const val twitter = "com.twitter.sdk.android:twitter-core:3.1.1@aar"
+        }
+
+        object Miscellaneous {


nit nit: shorten to Misc?

*Goes back to his own project to fix that too* 😆

samtstern · 2018-05-29T18:34:46Z

buildSrc/src/main/kotlin/Projects.kt

+    "check"(this, block)
+}
+
+private inline operator fun String.invoke(project: Project, crossinline block: Task.() -> Unit) =


Can you explain this?

Yeah, I'm not super sold on this either, but I've seen similar DSL before: the idea is to override what happens when you call "someString"(). In this case, we're saying you can do this:

"someTask"(myProject) { // Do stuff with the task }

I don't mind killing it if you're dumbfoundedly confused like how I felt the first time I saw that kind of DSL. 😁

samtstern · 2018-05-29T18:35:49Z

firestore/src/main/java/com/firebase/ui/firestore/paging/PageKey.java

@@ -20,7 +21,8 @@ public PageKey(@Nullable DocumentSnapshot startAfter, @Nullable DocumentSnapshot
        mEndBefore = endBefore;
    }

-    public Query getPageQuery(Query baseQuery, int size) {
+    @NonNull


Please update the PR title to reflect all the nullability stuff.

Will be moved into another PR

samtstern · 2018-05-29T18:36:34Z

internal/lint/src/main/java/com/firebaseui/lint/LintIssueRegistry.kt

@@ -6,5 +6,5 @@ import com.android.tools.lint.client.api.IssueRegistry
 * Registry for custom FirebaseUI lint checks.
 */
 class LintIssueRegistry : IssueRegistry() {
-    override fun getIssues() = listOf(NonGlobalIdDetector.NON_GLOBAL_ID)
+    override val issues = listOf(NonGlobalIdDetector.NON_GLOBAL_ID)


override val --> Kotlin is cool but also sometimes it's stupid. This is not an actionable comment, just being grumpy Java guy.

Awww, c'mon!? How can you not love overriding fields? It's so clean! ✨

samtstern · 2018-05-29T18:37:01Z

library/build.gradle.kts

+    api(project(":auth"))
+    api(project(":database"))
+    api(project(":firestore"))
+    api(project(":storage"))


Do we need common here?

We didn't have it before and Gradle should take care of downloading those kinds of transitive dependencies.

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2018-05-30T01:51:03Z

@samtstern Yup, it fixes the lint issues and I've merged from 4.0.1. 👍

samtstern · 2018-05-30T14:58:03Z

@SUPERCILEX LGTM but the one part I don't agree with is app --> sample. Especially since it makes this PR touch 100+ files. Let's just rename it back to app and then we'll merge it and see if we can get 4.0.1 going.

Signed-off-by: Alex Saveau <[email protected]>

…-kotlin-dsl # Conflicts: # constants.gradle

SUPERCILEX · 2018-05-30T15:11:30Z

SGTM, though do you think it should be done at some point? Personally, "app" has always felt vague.

samtstern · 2018-05-30T15:16:03Z

@SUPERCILEX I see app as a convention since it's what Android Studio calls the application module by default.

SUPERCILEX · 2018-05-30T15:58:24Z

Oh yeah, that makes sense since people will be familiar with it. 👍

SUPERCILEX · 2018-05-30T16:20:33Z

@samtstern huh, I broke the snapshot builds. 😭 I'll look into it this afternoon. Is standard publishing working?

SUPERCILEX added 4 commits May 16, 2018 23:26

Join the cool kids club (aka migrate to Gradle Kotlin DSL)

d0cba2f

Signed-off-by: Alex Saveau <[email protected]>

Merge remote-tracking branch 'upstream/master' into gradle-kotlin-dsl

316acc6

# Conflicts: # auth/build.gradle # constants.gradle # library/quality/quality.gradle

Fix merge mistakes

91678cd

Finalize refactor

f2f6188

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX requested a review from samtstern as a code owner May 28, 2018 21:10

Last few tidies

eb02072

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX mentioned this pull request May 28, 2018

No resource found that matches the given name #1317

Closed

SUPERCILEX changed the base branch from master to version-4.1.0-dev May 29, 2018 02:16

samtstern added this to the 4.0.1 milestone May 29, 2018

samtstern changed the base branch from version-4.1.0-dev to version-4.0.1-dev May 29, 2018 18:21

samtstern reviewed May 29, 2018

View reviewed changes

SUPERCILEX added 4 commits May 29, 2018 18:14

Fix merge mistakes

1114a43

Signed-off-by: Alex Saveau <[email protected]>

Fix broken tests

91e8b64

Signed-off-by: Alex Saveau <[email protected]>

Ignore debug variant of proguard tests

06a530e

Signed-off-by: Alex Saveau <[email protected]>

Fix minor lint issues

62d1ddb

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX mentioned this pull request May 30, 2018

Start adding nullability annotations to all public or protected symbols #1326

Merged

SUPERCILEX added 3 commits May 30, 2018 08:07

Address review feedback

27002fb

Signed-off-by: Alex Saveau <[email protected]>

Fix merge mistakes

3d0abc1

Signed-off-by: Alex Saveau <[email protected]>

Merge remote-tracking branch 'upstream/version-4.0.1-dev' into gradle…

8abf58b

…-kotlin-dsl # Conflicts: # constants.gradle

samtstern merged commit f7cbc1e into firebase:version-4.0.1-dev May 30, 2018

SUPERCILEX deleted the gradle-kotlin-dsl branch May 30, 2018 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Join the cool kids club (aka migrate to Gradle Kotlin DSL) #1321

Join the cool kids club (aka migrate to Gradle Kotlin DSL) #1321

SUPERCILEX commented May 28, 2018

samtstern commented May 29, 2018

samtstern left a comment

samtstern May 29, 2018

SUPERCILEX May 30, 2018

SUPERCILEX May 30, 2018

samtstern May 29, 2018

SUPERCILEX May 30, 2018

samtstern May 29, 2018

SUPERCILEX May 30, 2018

samtstern May 29, 2018

SUPERCILEX May 30, 2018

samtstern May 29, 2018

samtstern May 29, 2018

SUPERCILEX May 30, 2018

samtstern May 29, 2018

SUPERCILEX May 30, 2018

samtstern May 29, 2018

SUPERCILEX May 30, 2018

samtstern May 29, 2018

SUPERCILEX May 30, 2018

samtstern May 29, 2018

SUPERCILEX May 30, 2018

SUPERCILEX commented May 30, 2018

samtstern commented May 30, 2018

SUPERCILEX commented May 30, 2018

samtstern commented May 30, 2018

SUPERCILEX commented May 30, 2018

SUPERCILEX commented May 30, 2018

Join the cool kids club (aka migrate to Gradle Kotlin DSL) #1321

Join the cool kids club (aka migrate to Gradle Kotlin DSL) #1321

Conversation

SUPERCILEX commented May 28, 2018

samtstern commented May 29, 2018

samtstern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX commented May 30, 2018

samtstern commented May 30, 2018

SUPERCILEX commented May 30, 2018

samtstern commented May 30, 2018

SUPERCILEX commented May 30, 2018

SUPERCILEX commented May 30, 2018