-
Notifications
You must be signed in to change notification settings - Fork 515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SQL clause should persist adapted type #2067
Comments
@AlecStrong I'm thinking about working on this, but as it's a non-trivial change to the SQLDelight grammar, I'd be interested to hear your thoughts on this feature before I go ahead with implementing it 😄 |
Its really nice.
On Nov 11, 2020 2:33 AM, "Veyndan Stuart" <notifications@github.com> wrote:
@AlecStrong <https://github.com/AlecStrong> I'm thinking about working on
this, but as it's a non-trivial change to the SQLDelight grammar, I'd be
interested to hear your thoughts on this feature before I go ahead with
implementing it 😄
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2067 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQMHD34RAD6C4PMFQERQ4XLSPHEVZANCNFSM4S5ZE3ZA>
.
|
we could also make this the default behavior for columns with the same kotlin type and throw a runtime exception if the two adapters are different, something like fun selectBirthdays(): Query<T> {
check(adapter1 == adapter2)
...
} I would be surprised if that behavior would be undesirable. thoughts? |
I really like your idea, as it keeps the SQLDelight grammar simple, and the user doesn't have to be aware of typealiases to receive the benefits of stronger typed generated code. Also, and I think this is probably the biggest benefit going in this direction, is that it'd remove a class of bugs. For example, carrying on from the original issue comment, if the database initialization was something like this: test_database(
driver,
Adults.Adapter(birthdayAdapter = LocalDateColumnAdapter(format = "dd/MM/yyyy")),
Children.Adapter(birthdayAdapter = LocalDateColumnAdapter(format = "dd MM yyyy"))
) where a The only benefits that I currently see of typealiases over a So overall I think I'm siding with your proposal. Are you good with going in that direction? |
yea sounds good. agreed compile time safety is good, i usually err on the side of not making our own sql dialect |
@AlecStrong There's a potential regression with the "adapter equality" approach, where (for example) the I made an example to illustrate what I mean. |
even if you just do
that should return an Int and not an Age. So then the union would be a union of an |
Ah gotcha, thanks for the clarification! 😄 |
See #2056 (comment).
Currently, if we union two tables with each column having the same java type, the generated query interface will use the underlying java type, as it is possible that each column has a different
ColumnAdapter
, so the query implementation wouldn't be able to know whichColumnAdapter
to use when creating the adapted type when constructing the query interface. Ideally, what we would want is the union to produce the adapted type, iff theColumnAdapter
for the column in each table are identical.Example
Status Quo
People.sq
Birthdays.kt
Database Initialization
Proposal
People.sq
Birthdays.kt
Database Initialization
The text was updated successfully, but these errors were encountered: