Use managed & lazy properties in Gradle plugin extension

[3flex]

This is an opportunity for SQLDelight 2.0.0 IMHO.

Configuration for the Gradle plugin is currently managed entirely with the extension (as opposed to the task), which I've updated in two ways:

1. Simplified the implementation. This is a breaking change for Kotlin & Groovy DSL users, but a mechanical update to build scripts. Resolves a planned Gradle 8 deprecation (no more ConfigureUtil).
2. Switched to the lazy property API in the extension and SqlDelightDatabase. This is a breaking change for Kotlin DSL users, but not Groovy. Gradle is encouraging its use and considers use of getter/setter methods (including those generated by Kotlin) to be legacy. Again, a simple update to build scripts is required.

All users will have have to wrap database declarations in a databases {} block. Kotlin users will have to use create("databaseName") to create a database, and use .set on properties:

sqldelight {
  databases { // new wrapper
    Database {
      packageName = "com.sample"
    }
  }
}

sqldelight {
  databases { // new wrapper
    create("Database") {
      packageName.set("com.sample") // packageName is now a Property<String>
    }
  }
}

I started going further by migrating to lazy properties in the tasks as well, but as these are not documented APIs, and it's more complex to achieve, I think this can be done after 2.0.0 is released as time and motivation allows.

If this is acceptable I will update docs & generated code snippets such as this, as part of this PR: https://github.com/cashapp/sqldelight/blob/d21d2ee310a9e96f5227f10d4920eafa27806914/sqldelight-gradle-plugin/src/main/kotlin/app/cash/sqldelight/gradle/android/PackageName.kt#L20-L24

[AlecKazakova]

what's the constraint that requires a databases { } block? I assume its because the extension switched to an interface but I'm not sure what that was necessary. I don't mind the property stuff but I do mind the extra block

[3flex]

There's no constraint requiring databases {}, it's just convention in Gradle to have a container to manage instances of a specific type - think tasks {}, repositories {}, sourceSets {}, buildTypes {}, productFlavors {}... I'd argue the semantics are a little clearer this way too. The old DSL can be restored though if you feel strongly about it.

[AlecKazakova]

makes sense. semantically I think of sqldelight {} as the container DSL, though technically thats not actually how its implemented. I'd prefer to keep that way if for nothing else than to reduce the amount of changes in the PR and to upgrade to 2.0, but I also just personally like the look of it

[3flex]

I'm refactoring to restore the old API & keep the use of lazy properties, but there's a snag with the methodMissing piece.

After changing to val packageName: Property<String>, this fails:

sqldelight {
    MyDatabase {
        packageName = "app.cash.sqldelight.hsql.integration"
    }
}

When that's passed to ConfigureUtil.configure(closure, database) it fails because the the packageName backing field is final. The methodMissing trick bypasses some of Gradle's "magic" which would normally decorate SqlDelightDatabase so that the closure passed in will be mapped to the properties with the Property type (and other lazy types).

TL;DR: would this be OK?

sqldelight {
    database("MyDatabase") { // <- change here
        packageName = "app.cash.sqldelight.hsql.integration"
    }
}

It's still breaking for Groovy DSL users though. I'm not sure if there's any way around it - or you can just leave it as is and close the PR, but if ConfigureUtil is removed in Gradle 8 as planned, there might be breaking changes then, instead of now.

[AlecKazakova]

could we use a kotlin property with a custom setter?

private val package: Property<String>

var packageName: String
  get() = ...
  set(field) = package.set // or whatever the right API is

[3flex]

Could do, but it's getting away from what I'd intended with this PR, which was:

1. Drop legacy getters/setters in the extension and use the recommended Property API (which made sense to do for 2.0.0 given it's a breaking change)
2. Simplify the Gradle plugin implementation

If we're just adding a private property but keeping the DSL the same it's just adding complexity and I don't see any upsides. Also this isn't just affecting packageName, but all the extension properties that changed type, I just used that as an example, so there'd be a whole lot of private properties to be added.

I'll close for now - I can't offer much if the API has to stay the same (which is absolutely understandable! It ain't broke...)

[3flex]

Reopening, see #3464 and #3467

[hfhbd]

@3flex What is the status? Could you resolve the conflicts and make it ready? :) Personally, I want to move to standard Gradle without any custom implementations/workarounds.

[3flex]

Ready for review, except docs, which I can update once the DSL is agreed.

[hfhbd]

I know this will be a breaking change for Groovy users too but could we add the database container to be more "gradle-like" and drop the "hacks" (methodMissing)? Edit: You changed it using force-push, ok.

[AlecKazakova]

I'm converted, lets get some docs in before merging?

[3flex]

Rebased & docs updated (I think I've caught everywhere that had to be updated, let me know if I've missed anything).

[eygraber]

Is it worth mentioning this change in the release notes?

[hfhbd]

Yeah

[hfhbd]

Updated

[eygraber]

Could probably use some indication which snippet is for Groovy and which is for Kotlin :sweat_smile:

[3flex]

In the Groovy example you'd use packageName = 'com.sample' instead of calling set explicitly (which also reduces the changes required in builds scripts).