Closed kayrein closed 1 year ago
This is great, a long standing feature getting implemented! I have a look at the implementation and although it looks correct to me, I have some concerns with the public API that I'd like to share.
Even though the dot.WithOptions(opts).Query(...)
seems to be more ergonomic, I think that WithOptions
should return an error when the parsing fails; as opposed to wait for lookupQuery
. If my queries have an error I'd like to know as soon as possible.
I'm not sure about naming. Options
suggest something that you can choose from, like a config. I'd use WithData
, WithParams
or even WithArgs
Finally, instead of receiving a map[string]interface{}
(which now it looks like any
is used instead of interface{}
), I'd pass just any
, which is what .Execute
accepts.
In https://github.com/qustavo/dotsql/pull/27/commits/522d69598083d65df5cbd5b4a7ea28815de1ffcf I made it parse all the queries when you load them - so if you load your dotsql files at application start and have a syntax error in one of your queries it will fail at start rather than when you try to call the query. Which is pretty fast! I did add a test to assert this behaviour though.
But the WithData(...) call cannot fail because it doesn't yet know which query you're executing - it would have to try each query with that data.
Have acted on the other comments though :)
Any thoughts @qustavo?
@kayrein added you as a member, do you think you can cut a new release including this?
Introduce support for text interpolation