sqlc-dev
fix(sqlite): introduce "update from..." to the parser
This PR closes #3132 by adding the update from... syntax to the sqlite update_stmt parser.
Was tested locally with several different queries.
Please let me know if there's anything more required to get this in or if you have any comments.
Hey @kyleconroy,
I wanted to follow up on this PR, and see if there’s anything I can do to help move it toward merging. If there’s any feedback or additional work needed on my end, please let me know - I’d be happy to make adjustments to ensure it aligns with sqlc's vision.
Thanks! Ido
Hey @kyleconroy 👋 I saw that you force pushed - I went ahead and fixed the failing test, please let me know what else is needed to move forward with this.
Hi again @kyleconroy , any chance you can find the time to review and help merge this PR? thanks! 🙏
Hi again @kyleconroy is there anything I can do to expedite getting this fix upstream?
I'm waiting on this PR too. I had to clone this PR and host it myself so I can continue developing my backends. It's impossible to use SQLC otherwise.
@IdoKendo Great stuff!
I'm not a maintainer here, but I was just reading through the diff and one thing that struck me was there's no proof test-wise that the changes work. Do you think it'd make sense to get a basic internal/endtoend/testdata/ test into place that checks against this? They're relatively easy to write.
@kyleconroy Hey! This one might make a good addition for the next sqlc release cycle. There's a great many bugs left in the SQLite side of things, and this is definitely one of the annoying ones. For context, UPDATE ... WITH is supported by Postgres as well and has worked in sqlc for ages.
@IdoKendo Great stuff!
I'm not a maintainer here, but I was just reading through the diff and one thing that struck me was there's no proof test-wise that the changes work. Do you think it'd make sense to get a basic
internal/endtoend/testdata/test into place that checks against this? They're relatively easy to write.
Hey @brandur, thanks!
You're right, I only tested the changed manually against the use-case mentioned as well as against previous use-cases to ensure it didn't break anything :)
I would be happy to add some tests into place but I'm afraid this PR has been sitting here too long and I need to get some guarantee that it is going to get merged upstream before I invest any more time and effort into it. As you can see in the history I tried to get some attention several times and got shunned.
If this is something the maintainer is willing to merge I will add tests to get it into the next release :)
Cheers
I would be happy to add some tests into place but I'm afraid this PR has been sitting here too long and I need to get some guarantee that it is going to get merged upstream before I invest any more time and effort into it. As you can see in the history I tried to get some attention several times and got shunned.
@IdoKendo 100.0% reasonable, and I'd request the same. Thanks.