-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Allow casting when using named bindings. #1890
Conversation
Hi, thanks for looking into this I'll give more careful look to the implementation a bit later.. On the first glance looks like you have used template strings in tests... sadly we still cannot use them in tests, since they are ran also with node 0.12. So this causes test failure. Also if I understood correctly your implementation checks just against There is a bug report about the same issue, which has also proposal for the solution to the problem that will work in more general way to all colons in the query #1887 (comment) |
@elhigu Thanks for looking into this. Edit: I realized timestamps have colons and possibly any other raw input strings. |
@mathewdgardner do you think you could change the implementation to work so that only those named bindings and converted to positional which are found from bindings object? |
@elhigu I'll take a stab at it. |
@elhigu Let me know if this implementation works you or if I need to squash. I expanded the regex to either match a named binding identifier that is not followed by a casting or a named binding value otherwise. Casts will still be picked up by the regex but it will only use the given bindings for replacement. If a binding given is undefined, it will still pass it along; that behavior is unchanged. |
So... casting We could make special cases for Would be great to add a paragraph about how named bindings work in these special cases where syntax is a bit unambiguous. Code looks good. Consider those last comments if you like to do some more tuning and let me know. If you feel this is ready in current form I can already merge this in. |
Awesome thanks a lot! |
added issue about documentation |
* Allow casting when using named bindings. * Remove template string in test. * Only replace matches that are member to the bindings object. * Slight refactor for tests. * Simplify regex. * Allow casting on named identifiers.
I have had lots of trouble using named bindings when writing raw queries in the past but recently I found that most of my trouble occurs if I am casting. This PR expands the regex used for searching for identifies and values and adds a little bit of logic around it.
This may be a naive approach but I'm open to critique.
Before Examples:
Currently, this will yield an error complaining about undefined bindings. What's happening behind the scenes is
::TEXT
is being picked up as a named binding but of course it is missing from the given bindings object.Currently, this will result in
:foo:
being thought of as an identifier instead of a value being given and casted.After some digging I found that this undocumented approach works in the current version:
While this does not:
After Examples:
These work as expected.
I mostly work on Postgres and so my knowledge of other RDBMS's is very limited. As I mentioned above I am open to suggestions for improvement.