Skip to content

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

Merged
merged 5 commits into from
Oct 14, 2013
Merged

expose SqlString.format #523

merged 5 commits into from
Oct 14, 2013

Conversation

whoughton
Copy link
Contributor

#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?

@dresende
Copy link
Collaborator

This forces people to know all the arguments (like timezone), if not using a Connection (there's a Connection.format). Is this what you mean?

@whoughton
Copy link
Contributor Author

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).

@tellnes
Copy link
Collaborator

tellnes commented Jul 1, 2013

I think this would be nice if we can make it work without any configuration (like creating a Connection).

@dresende
Copy link
Collaborator

dresende commented Jul 2, 2013

I think it works without passing the other arguments, people just have to know the default behaviour.

@whoughton
Copy link
Contributor Author

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;
};

@dresende
Copy link
Collaborator

dresende commented Jul 4, 2013

@felixge what do you think?

@felixge
Copy link
Collaborator

felixge commented Jul 9, 2013

@dresende I see no harm in exposing this given proper docs.

@tellnes
Copy link
Collaborator

tellnes commented Jul 13, 2013

Related #472

@whoughton
Copy link
Contributor Author

Unsure if there is anything I need to do at this stage other than wait.

@dresende
Copy link
Collaborator

Please just add some documentation about this to the Readme.md file.

@whoughton
Copy link
Contributor Author

My apologies, I lost track of this, will work on getting it added this weekend.

@whoughton
Copy link
Contributor Author

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.

@felixge
Copy link
Collaborator

felixge commented Oct 13, 2013

@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.

@whoughton
Copy link
Contributor Author

Grrr, sorry, looks like the upstream pull didn't work quite right or something. Looking into it now.

@whoughton
Copy link
Contributor Author

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?

whoughton added a commit that referenced this pull request Oct 14, 2013
@whoughton whoughton merged commit d3a8d98 into mysqljs:master Oct 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants