canonical / sqlair

Friendly type mapping for SQL databases
Apache License 2.0
17 stars 9 forks source link

Redistribute Type Bind responsibility #127

Closed Aflynn50 closed 9 months ago

Aflynn50 commented 10 months ago

Move the responsibility of generating SQL to the Bind Inputs stage. This will allow SQL to support IN expressions where the SQL that is generated depends on the input arguments.

letFunny commented 9 months ago

I really like the latest changes of moving the bindTypes to a method on both input and output expressions. However, now that I see the code I personally disagree with removing the expression types and changing it to any. The new signature:

func (b *bypass) bindTypes(argInfo typeinfo.ArgInfo) (any, error) {

gives me little information about what the return is. I have to either look at the implementation or look at the comments (which can get outdated). Before it would have been:

func (b *bypass) bindTypes(argInfo typeinfo.ArgInfo) (*typedExpression, error) {

I my personal opinion, I think the added readability is worth it even if it means having a dummy interface as a substitute for a proper enum type. I would like to know your thoughts on this tradeoff, do you think it is not worth it?

Lastly, this is all based on the latest commit, if we add generics the code might look different enough that it will not be applicable.