generated projections do not use package name

lnhrdt commented 1 year ago

SQLDelight Version

2.0.0-alpha05

Operating System

macOs

Gradle Version

7.6

Kotlin Version

1.8.10

Dialect

app.cash.sqldelight:postgresql-dialect:2.0.0-alpha05

AGP Version

No response

Describe the Bug

After upgrading from 1.5.5 to 2.0.0-alpha05, generate projections no longer have the configured package name. However other generated objects do (i.e. the database interface).

for example, a gradle plugin configuration like this:

...

configure<SqlDelightExtension> {
  databases {
    create("ThingDatabase") {
      dialect("app.cash.sqldelight:postgresql-dialect:2.0.0-alpha05")
      packageName.set("com.sample")
      ...
    }
  }
}

Properly produces a ThingDatabase in package com.sample but the data classes generated for a table have no package.

Stacktrace

No response

Gradle Build Script

No response

hfhbd commented 1 year ago

As a workaround move the sq files into sqldelight/com/sample

lnhrdt commented 1 year ago

As a workaround move the sq files into sqldelight/com/sample

Thanks @hfhbd, but I already have had my sq files in a package (on the filesystem). Been that way since I had things working properly using 1.x versions.

Perhaps that's the mechanism that's not working now. I assumed it was the packageName configuration but perhaps it's the package in the folder structure that stopped affecting generated files.

hfhbd commented 1 year ago

Do you have a sample with the workaround?

Yes, the behavior did change, see my PR and the reason: https://github.com/cashapp/sqldelight/pull/2755#issuecomment-1005157722 I didn't expect this feedback (https://github.com/cashapp/sqldelight/issues/3423 and https://github.com/cashapp/sqldelight/issues/3346) so I think we have to fix it somehow.

lnhrdt commented 1 year ago

@hfhbd sure, I just made this for you: https://github.com/lnhrdt/cashapp-sqldelight-issue-3947

Check it out, but the punchline is in Application.kt.

Let me know if I'm configuring something incorrectly in my projects but if I were to restate the crux of my issue, I find it very confusing that:

databases use the packageName config in gradle to get their package
while query files (.sq) must reside in a folder to get a package, despite the .sq syntax not having kotlin package statements at the top of the file.
and projections generated (in my case from migrations) just don't get a package at all.

hfhbd commented 1 year ago

Thanks, so yeah I agree, we should fix the current behavior.

The projections are not stored in a package because the migrations files are not stored in a package/folder too.

lnhrdt commented 1 year ago

because the migrations files are not stored in a package/folder too

Wild! I just tried moving my migrations into subdirectories to mimic a package and it did apply a package to the generated projections.

As y'all design this library, my feedback would be that to me it's counterintuitive for things that aren't java/kotin files to get a package from a directory (and without a package statement in the syntax of the file). As someone trying to learn sqldelight and coming from other more popular (and older) database technologies, I'm realizing now that I've been thinking of sqldelight's .sqm migration files as more akin to regular SQL files and expected them to behave more simply. I had assumed that if there was any "magic" happening as part of the code generation, it would be configured only in the gradle plugin. Although, I realize this sqldelight paradigm is very different in some ways. If it's well documented I think new conventions could be learnable.

Super excited to see how this shakes out for 2.0.0.

morki commented 1 year ago

For this reason I would like to see some Gradle property like packagePrefix to be not enforced to stack .sq and .sqm files in long hierarchy of folders to reproduce package name. Or maybe package directive like import in those files.

lnhrdt commented 1 year ago

@morki nice, I think either of those options would be more intuitive.

The most confusing part for me about the current the packageName config is that it's not immediately clear which generated files it affects. So I incorrectly assumed it affected all generated files. Perhaps it could be named something more specific like databaseClassPackage and then allow package directives for .sq and .sqm files as you suggested. That would allow users the flexibility to have different generated classes appear in different packages (although I don't anticipate I'd use that in my projects).

Slightly off topic but I think still worth mentioning as y'all think through this, I don't know about other users but the main reason I'm so particular about the package names with sqldelight is really just as a workaround for #1333. Since I can't make the generated classes internal to a module, I'm scoping them with a package to help maintain a convention of not using generated classes outside of the module where they are generated.

lnhrdt commented 1 year ago

Today while refactoring some code I saw the latest documentation now describes packageName as "Package name used for the database class." This helps a lot (thanks @dellisd, I saw you on the commit). I'm tempted to close my issue here but thought I'd first poke one last time at the design:

The way sq/sqm files get a package from their directory still feels unintuitive to me. I think @morki's suggestion to add a "package directive like import" to sq/sqm files akin to java/kotlin files would make this more intuitive. Any chance of that happening?
Even with the description of packageName in the docs, I still think the name is misleading and would be easier to navigate if it had a more specific name. I'm guessing the "name" suffix was only added to the property so it didn't clash with the package language keyword. So perhaps databaseClassPackage would be most clear?

cashapp / sqldelight