kotlin: remove runtime dependency #774
Conversation
mightyguava
force-pushed
the
kotlin-runtime
branch
from
3197b53 to
7c906cd
Compare
The runtime in sqlc-dev#368 turned out to be a bad idea. It's pretty much unnecessary and adds quite a bit to the complexity of the generated code. It's _possible_ a runtime would be useful in the future. At that point we can add that back in
mightyguava
force-pushed
the
kotlin-runtime
branch
from
7c906cd to
cd3919a
Compare
| @@ -36,91 +32,71 @@ ORDER BY name | |||
| class QueriesImpl(private val conn: Connection) : Queries { | |||
There was a problem hiding this comment.
Personally I try to avoid the Impl suffix because it doesn't say anything about the implementation which feels like a missed opportunity. Maybe GeneratedQueries?
(I assume this is generated right?)
There was a problem hiding this comment.
But it’s the Java way! I agree. But since naming didn’t change in this PR I’d like to leave it for a follow up if you feel strongly? Also all of this code is generated. Maybe RealQueries which isn’t anymore meaningful but seems like the Kotlin way.
I think you’ve been corrupted by Go
There was a problem hiding this comment.
The Impl suffix is afaict never used in the JDK so while it's common in some Java projects it's not really a "sanctioned" convention. :-) I don't care about it strongly but something to think about.
| val results = stmt.executeQuery() | ||
| val ret = mutableListOf<BooksByTagsRow>() | ||
| while (results.next()) { | ||
| ret.add(BooksByTagsRow( |
There was a problem hiding this comment.
I don't know how JDBC implementations are implemented these days but the JDBC API has been built to encourage "streaming" where not the entire result set has to be sent over the wire before it can start being processed by the application. This implementation seems to break that. I think you can build a list implementation on top of ResultSet using AbstractSequentialList quite easily? You would have to convert the checked SQLException to unchecked exceptions though so that might be a problem...
There was a problem hiding this comment.
Actually, doesn't Kotlin not have checked exceptions?
There was a problem hiding this comment.
The "streaming" method is supported in Go too. It's a lower level usage that sqlc doesn't currently handle because it wants to abstract away row parsing. I think if @kyleconroy adds a command for the Go version, like :execstream, we can replicate it for Kotlin. Otherwise this should match the Go impl to always parse the results first.
And no, Kotlin doesn't have checked exceptions...
| ret | ||
| } | ||
| if (results.next()) { | ||
| throw SQLException("expected one row in result set, but got many") |
There was a problem hiding this comment.
Do you support a Book? type such that the generated code can return a null instead of throwing an exception?
There was a problem hiding this comment.
So that's an interesting question. In Go one usually would get a sql.ErrNoRows when querying for a single row but the resultset is empty. In Kotlin this throws an exception that needs to be caught. Except there's no such canonical exception (I think), so we need to create one and stick it in a runtime library.
Or, maybe the :one and :many commands for sqlc which return 1 row or a list of rows respectively, should actually continue to return a RowResults and ListResults class? The RowResults could have .get() which throws and getOrNull() which does not. While the ListResults class could have a getList() to get the full list or next() to get the next item in the list.
There was a problem hiding this comment.
Oooo or return an Iterator() 😱
| stmt.setString(1, title) | ||
| bookId: Int) { | ||
| conn.prepareStatement(updateBook).use { stmt -> | ||
| stmt.setString(1, title) | ||
| stmt.setArray(2, conn.createArrayOf("pg_catalog.varchar", tags.toTypedArray())) |
There was a problem hiding this comment.
I assume this doesn't work on mysql? (Your other PR?)
There was a problem hiding this comment.
Nope. This field was changed to a comma delimitered varchar in the mysql version
| import sqlc.runtime.ExecuteQuery | ||
| import sqlc.runtime.ListQuery | ||
| import sqlc.runtime.RowQuery | ||
|
|
||
| interface Queries { |
There was a problem hiding this comment.
This API feels a lot more idiomatic now!
The runtime in #368 turned out to be a bad idea. It's pretty much unnecessary and adds quite a bit to the complexity of the generated code. It's possible a runtime would be useful in the future. At that point we can add that back in