-
Notifications
You must be signed in to change notification settings - Fork 858
Add variant metadata for sva
sentences per #4750
#4859
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
Add variant metadata for sva
sentences per #4750
#4859
Conversation
Size Change: +2 B (0%) Total Size: 6.87 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @moz-kathyreid, thanks for the migration. I tested it and it works somewhat. If you run each individual update statement separately it works but if you try to run all at once it crashes. This is basically what you did by putting all update statements into one query. You have to run each update statement as a separate query, i.e.
export const up = async function (db: any): Promise<any> {
await db.runSql('update one')
await db.runSql('update two')
...
}
then it will work as expected 👍
Don't review this yet, it's still failing on |
sva
sentences per #4750sva
sentences per #4750
…ully Parse error was due to missing `"` and a doubled `""` ...
sva
sentences per #4750sva
sentences per #4750
This is now ready for review. The way I approached this was:
I believe this can now be reviewed and merged, and apologies for the stuffing around, I should have been more thorough the first time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, ready to go 🚀
Pull Request Form
Type of Pull Request
Testing done
docker compose up
to validate.ts
file is syntactically correct, however it has no impact on my dev db because thesva
sentences are not loaded there (were imported in bulk). So I can't guarantee that the update works as intended, just that the SQL itself is syntactically correct.Acknowledging contributors
Many thanks to @razmikb for doing the hard work of assigning each sentence to a variant in the spreadsheet.