Skip to content

Conversation

@arkenidar
Copy link

@arkenidar arkenidar commented Jun 17, 2016

handle 'stringable' objects in query() method, besides strings and query objects
(automatic to string conversion, then handling as a string)

I would like to add for convenience an automatic toString() conversion of a parameter that could be a string.
The is another use case of having a parameter with a config object and in that case the config object is handled as usual.
So we have a sum total of 3 use case handled: strings, stringable objects and config objects.
The rationale of this patch is having the toString conversion in one place inside the query method call instead of having to call many times toString (at every query method call).
So this patch would allow to reduce replication of "toString" code (when calling query method).

@brianc
Copy link
Owner

brianc commented Jun 20, 2016

Hey @arkenidar I really appreciate the thought you put into this! I think the direction you're taking sounds good for your codebase, but I have to say I'm -1 for having a .toString() call on a query coerce it into an executable query.

The main reason is this doesn't encourage (or even support) the use of parameterized queries. In the .toString() scenario where do the parameters go? Also, it can end up w/ weird edge cases as people pass things to query which have a custom .toString() and unexpectedly get executed. The whole "principal of lease surprise" coming in to play here. I think probably the best approach if you want this behavior in your own modules is to have a custom query method somewhere in your codebase that does this sort of introspection and passes the "proper" object into the actual Cient#query method on a client. That method is already overloaded past my level of 100% comfort, so overloading it even further isn't something I'm likely to support unless there's no way the functionality can live in a separate module.

Thanks again for the work you put in here! More than happy to help you set up an external module to do this sort of thing for you!

@brianc brianc closed this Jun 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants