Kotlin prototype #300
Kotlin prototype #300
Conversation
|
Ooh, I have some ideas around DRYing the 2 languages by taking advantage of embedded structs. Better left for a follow up though, and they do say the best time to dedupe is when you have the 3rd copy. |
internal/dinosql/ktgen.go
Outdated
|
|
||
| // HACK: jdbc doesn't support numbered parameters, so we need to transform them to question marks... | ||
| // But there's no access to the SQL parser here, so we just do a dumb regexp replace instead. This won't work if | ||
| // the literal strings contain matching values, but good enough for a prototype. |
There was a problem hiding this comment.
FWIW you don't need a full parser, just a lexer that correctly tokenises strings and comments. Though that in itself might be a challenge across multiple dialects :. OTOH it would be just cleaner to do it with an AST.
|
Wow! The fact that you were able to hack this together is really impressive. And here I thought TypeScript would be the second language we support.
There are a few things that will need to change before we can merge in Kotlin support. The first is a new version of the configuration file that supports multiple language backends. You already noticed that most of the package options don't make sense for Kotlin. I'm tracking that in #302 The interface between parsing, type-checking, and code-generation is still in flux. I don't think it will be frozen any time soon. Have a package for each language is probably a good approach going forward.
That's fine, as the first version that ships will be marked experimental. Before we expose it to everyone, we'll need to add MySQL support.
I haven't touched Java in years and have never written Kotlin. It would be fantastic if we could get a small group of opinionated Kotlin developers to look at the generated code and make suggestions. Kotlin also has a more featureful type system than Go, so it's possible that the generated code can be more clever.
We can do this safely and correctly when parsing queries. We have the full query AST and a means to edit both the AST and the input SQL. So, where does that leave us? I think it makes sense for this to live in your sqlc fork for the time being. Once the configuration file version has been updated, we can then reassess. In the meantime, I'd focus on porting the tests in Thanks again! I'm really excited about this landing in the future. |
Yup, MySQL is my number 1 want aside from this. I want to contribute there too, but it seems like there are already plenty of folks on it! I know
Sure, I can get some folks on my end. Do you know anyone who might be interested as well?
Already ported last night! Will get it running on CI and add some more tests. |
|
mightyguava#2 adds CI |
|
Ported over all the tests! Everything passes except a few |
|
Nice job putting this together 👍. As far as MySQL support, when the time comes I don't think it'll be too much work to satisfy the interface you've put together. It looks like the current pattern we have of using an " To that point, I'm thinking it'd be better to put the Kotlin generation templates in their own package, then have the |
|
Thanks! Kotlin generation is already in its own package mightyguava#3. I’m not planning to add more commits to this PR. Instead, I’m going to use the TODO at the top to link to PRs on my fork. The “kotlin-master” branch on my fork will ultimately contain all the commits to eventually merge back. Yeah the Generatable interfaces do seem to work so far. I think there’s a bit more common functionality we can extract out, that can happen later when we move the Go generator out. |
Actually I figured out how to do the query rewriting. #306. Would like to merge this if possible to avoid future merge conflicts, it's very nuanced |
The main blocker for merging is #302, "Get some Kotlin folks to review generated code", and a decision around experimental features. My feeling is that we should just document that the generated Kotlin code is experimental in the README and warn users that it may change. |
|
I’ve asked @alecstrong (maintainer of SQLDelight), @ryanhall07 and @tirsen (2 engineers experienced with JDBC and Kotlin) to take a look at mightyguava#6. W.r.t identifying this as experimental, putting it in the readme sounds great. If we want to go a step further, we could make the config option be Let me know when you reach a decision on #302. |
|
going off the source generated here can the params be inlined? data class UpdateBookISBNParams (
val title: String,
val tags: List<String>,
val isbn: String,
val bookId: Int
)otherwise lgtm |
|
LGTM for the kotlin generated code in mightyguava#6 |
done: mightyguava#8 |
|
LGTM! |
|
@mightyguava The configuration changes are ready to be tested. Can you rebase this on the Once you've confirmed that this works, I'll merge Also, I'm not sure if you're following along to #150, but you can ignore MySQL support for Kotlin. We're going to switch parsers, which means the interface for MySQL will probably change. |
Sweet! Will do that tomorrow or Friday.
Yup, I proposed the parser switch actually and have been active in that thread. I look forward to full MySQL support! Separately, I sat down with @swankjesse today to do a deep code review and we found some good improvements to make to the generated Kotlin code. So this introduces a couple questions:
|
Yep, that plans sounds great. Let me know when you've decided on the repository name and I'll go ahead and create it. |
|
@kyleconroy I've updated this branch with the latest changes and retargeted the PR against
|
| files, err := dinosql.Generate(result, combo) | ||
| var files map[string]string | ||
| var out string | ||
| if sql.Gen.Go != nil { |
There was a problem hiding this comment.
The configuration format supports multiple language packages per package. This if statement means that only Go or Kotlin will be output. Code for both languages should be able to be output in a single pass.
There was a problem hiding this comment.
I believe this is correct. See lines 67-80 where I permute (engine, language) into the outPair struct. The parser needs to be run once per language because kotlin needs to run the parser with different options. I split out the parser changes to #306 earlier if you want to look closer there (though it's squashed into this PR and is outdated)
There was a problem hiding this comment.
Yep, I missed the fact that you were creating multiple output pairs. Looks good!
| } | ||
| return nil, true | ||
| } | ||
| return &kotlin.Result{Result: q}, false |
There was a problem hiding this comment.
Always returning a Kotlin result here seems strange. Is this just so that you can't generate MySQL code for Kotlin?
There was a problem hiding this comment.
Yeah, that, and me trying to minimize my changes to the generation code in fear of nasty merge conflicts. Happy to change if you can suggest a more elegant way of doing it. (check the lang here?)
There was a problem hiding this comment.
I think this is okay for now, but something we'll probably want to change once we land a third language backend.
|
The runtime repo has been created https://github.com/kyleconroy/sqlc-kotlin-runtime. Config-v2 has also been merged into master, so this should be good to go. |
|
Sweet, let's merge it. I'll work on the changes to use the runtime next week |
|
I've changed the base back to master. You'll need to fix some merge conflicts. I'm also planning on releasing v1.0.0 in the next few days. I'd like this to be merged in for that, but I'm planning on disabling Kotlin support in that release and then turning it right back on. The reason is that I'd like the v1.0.0 release to only support version 1 of the configuration format. |
|
Done! Could you merge this PR as soon as you can? I spent almost an hour doing this merge, plus another hour doing the previous merge. It sucked. Multiple conflicting changes were made recently that could have been avoided. The major ones were:
I understand how hard it is to be a maintainer, and I've done my best to respond ASAP to your comments and make changes as you requested to make it a bit easier. Please be mindful that it's also hard on the contributor to make large, meaningful changes. Resolving merge conflicts is not a good use of anyone's time, especially when it could have been easily avoided with better communication and timing. |
Done!
I completely understand this and want to thank you for getting this change over the finish line.
I apologize for making your life more difficult. We've all dealt with merge conflicts and nobody enjoys fixing them. Thanks again for all the amazing work |
Updated TODO:
authorsCI for Kotlin mightyguava/sqlc#2booktestandondeckPort Go tests, fix enums, use List instead of Array mightyguava/sqlc#4$1to?in query parsing instead of with a HACK in code generation rewrite numbered params to positional params mightyguava/sqlc#5I wanted to learn more about how sqlc works, so I took a stab at #287 anyways to see how hard it would be.
Here's a prototype implementation for Kotlin. It's pretty rough around the edges but tests pass, and I was able to generate and compile all of the existing examples in Kotlin.
authorshas tests written in Kotlin to confirm that the generated code works. I haven't gotten to the rest. The sqlc codebase is pretty easy to work in! I'm impressed.I basically just copied
gen.gointoktgen.goand renamed all the functions. It should really be its own package, but that can come if this PR actually has a chance of getting merged 😄While the code follows the same patterns as the Go generator, there were enough changes that I'm not sure much code reuse is possible.I split out the generated examples and a working gradle project because it would pollute this PR too much. They can be viewed here: mightyguava#1
Things that work:
Things that compile but I haven't tested:
Things that don't work:
Scan()interface in JDBC to support arbitrary types, so this'll require generating code to callAdapterclasses for individual types. Not hard, probably, just didn't seem worthwhile exploring in a prototypeThings that work differently:
?as placeholders. I had to write aHACKto convert things like$1to?. This is buggy and terrible. It also means that you can't use$1multiple times in the same query to reference the same value. If the AST was available in the generator part then I can probably figure out how to do something better.QueriesImpl.ktfor the queries instead one for each sql file. This is because you can't have a class span multiple files in Kotlin.EmitPreparedQueriesoption is likely unneeded here.Things to think about:
sqlcgenerates is boilerplate). I also just don't like writing Java.Please take a look! While I'd love for this code to get merged, I understand if you don't want to take on the additional maintenance burden that another language would add. Though, I wouldn't mind helping to maintain it.