Re-write Blueprint Target Insertion using QueryBuilder #9381
Conversation
smklein
changed the title
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
Overall, this is lovely. I had some minor suggestions, but most of them aren't important.
| .sql(") IS NULL, '") | ||
| .sql(NO_SUCH_BLUEPRINT_SENTINEL) | ||
| .sql( | ||
| "', \ |
There was a problem hiding this comment.
turbo nit: it looks kinda weird to me how the comma is not indented to where the previous expression would be, here...
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn insert_target_query_sql_snapshot() { |
There was a problem hiding this comment.
nit, take it or leave it: most of the other tests of this form i've seen are named expectorate_{query}, maybe we should make them consistent?
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn insert_target_query_is_valid() { |
There was a problem hiding this comment.
similar nit: most of these tests are named explain_{query}; should we make this consistent? also, the expectorate test also checks for the query's validity...
| let _ = query | ||
| .to_query() | ||
| .explain_async(&conn) | ||
| .await |
There was a problem hiding this comment.
nit, take it or leave it: i find it useful to also eprintln! the EXPLAIN output in these, so that you can run cargo nextest run {name_of_test} --success-output final if you want to see the query plan.
also, the other EXPLAIN tests generally include assertions that there's no FULL SCAN in the explanation, is that worth having here?
| impl InsertTargetQuery { | ||
| fn to_query(self) -> TypedSqlQuery<()> { |
There was a problem hiding this comment.
take it or leave it: now that this is using QueryBuilder, it doesn't really need to be its own struct; we could just have a function that takes the query parameters and returns a TypedSqlQuery. i think it only had to be a struct in order to implement the QueryFragment trait, but now we don't need a trait imp.
it's up to you whether you think it's cleaner to just have a free function or not, though 🤷♀️
Hopefully this is a little more readable.