Default imports for certain classes in sq files is confusing

[veyndan]

For example, I think it's a bit confusing to have to specify an import for kotlin.Byte, but not kotlin.Short. The types that are automatically imported are the ones that SQLDelight will automatically make type coercions for (per the docs).

The following lines defines which imports we consider as not being required to be explicitly specified:

sqldelight/sqldelight-compiler/src/main/kotlin/com/squareup/sqldelight/core/sqldelight.bnf

Lines 88 to 97 in 5d939ca

	java_type_name ::= 'Boolean'
	| 'Short'
	| 'Int'
	| 'Integer'
	| 'Long'
	| 'Float'
	| 'Double'
	| 'String'
	| 'ByteArray'
	| parameterized_java_type

I think it'd be less confusing to either:

• Require explicit imports for the types specified above (e.g., requiring import kotlin.Short in the .sq file)
  • Pros:
    • No potential naming conflicts with table names, view names, etc.
  • Cons:
    • Requires users to comb through .sq files and add imports
    • Makes users imports section larger (though I don't think this actually matters that much with code folding)
• Or, have the same default imports that Kotlin does.
  • Pros:
    • Doesn't require users to comb through their .sq files and add imports
    • Natural to Kotlin users as we're not used to having to explicitly import kotlin.Byte, kotlin.Short, etc.
  • Cons:
    • Potential naming collisions with table names, view names, etc. (e.g., a table called "List" would collide with kotlin.collections.List) Even though I think it should be easy to know which one to choose depending on the context, it may require some work.

We could keep the default, but I think the idea of these "random" default imports that are currently in SQLDelight will make less sense when/if #2056 is implemented.

[JakeWharton]

Default imports are a feature for text editors, not IDEs. I am very strongly opposed to doing what Kotlin does.

[veyndan]

Makes sense to me

[veyndan]

Thinking about this issue further, I believe we would need to implement this in conjunction with #2056, because otherwise there would be a potential regression for users that use the fully qualified name of the eight special cased classes which are imported by default.

Here's a test to illustrate what I mean, where we have two Int adapters, but one of them requires an explicit adapter. By implementing this issue before #2056, we would remove the ability to provide any custom ColumnAdapter's for these eight types.

@Test fun foo() {
  val result = FixtureCompiler.parseSql("""
    |CREATE TABLE test (
    |  col0 INTEGER AS Short,
    |  col1 INTEGER AS kotlin.Short
    |);
    |""".trimMargin(), tempFolder)

  val generator = TableInterfaceGenerator(result.sqliteStatements().first().statement.createTableStmt!!.tableExposed())
  val file = FileSpec.builder("", "Test")
    .addType(generator.kotlinImplementationSpec())
    .build()
  assertThat(file.toString()).isEqualTo("""
    |import com.squareup.sqldelight.ColumnAdapter
    |import kotlin.Long
    |import kotlin.Short
    |import kotlin.String
    |
    |public data class Test(
    |  public val col0: Short?,
    |  public val col1: Short?
    |) {
    |  public override fun toString(): String = ""${'"'}
    |  |Test [
    |  |  col0: ${'$'}col0
    |  |  col1: ${'$'}col1
    |  |]
    |  ""${'"'}.trimMargin()
    |
    |  public class Adapter(
    |    public val col1Adapter: ColumnAdapter<Short, Long>
    |  )
    |}
    |""".trimMargin())
}