-
Notifications
You must be signed in to change notification settings - Fork 2.5k
expose SqlString.format #523
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
Conversation
This forces people to know all the arguments (like timezone), if not using a Connection (there's a |
I didn’t use Connection.format as I believe it requires a configuration object, and with a base exposed method you don't want to have to set up a config object/connection. It also seemed like the utility of exposing the core format with options ends up being a benefit to an informed user. I believe that ultimately stringifyobjects and timezone are non-required options anyhow (they have reasonable defaults). |
I think this would be nice if we can make it work without any configuration (like creating a |
I think it works without passing the other arguments, people just have to know the default behaviour. |
Copying from the other thread to show how I am using it right now: var prepare = function(sql, values) {
if (typeof values === 'string') { values = [values]; }
var query = mysql.createQuery(sql, values);
if (!query.values) { query.values = []; }
query.sql = mysql.format(query.sql, query.values);
log("query: %s", query.sql);
return query;
}; |
@felixge what do you think? |
@dresende I see no harm in exposing this given proper docs. |
Related #472 |
Unsure if there is anything I need to do at this stage other than wait. |
Please just add some documentation about this to the |
My apologies, I lost track of this, will work on getting it added this weekend. |
Have added the readme update after pulling in the latest code. Please let me know if you have any thoughts on the location or content of it. |
@whoughton not sure what happened, but your commits seems to contain a whole bunch of other stuff: https://github.com/whoughton/node-mysql/commit/f924d913fc53cbc7d19d9b7199517acbb0827b95 Gave you push access to you can merge it in yourself once you've cleaned things up. |
Grrr, sorry, looks like the upstream pull didn't work quite right or something. Looking into it now. |
Ok, I ended up making it messier than I am proud of, but it should be all fixed now. Does anyone want to double check it before I merge it in? |
#521
As noted here, just exposing the SqlString.format to the module usage, allowing users to create formatted Queries for use with other modules.
Unsure of what needs to be done documentation-wise if anything?