Skip to content
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

Conflict with pg-promise #91

Open
kay999 opened this issue Oct 31, 2016 · 4 comments
Open

Conflict with pg-promise #91

kay999 opened this issue Oct 31, 2016 · 4 comments
Milestone

Comments

@kay999
Copy link
Contributor

kay999 commented Oct 31, 2016

I'm using sql-bricks together with pg-promise (in fact I'm using sql-bricks-postgres, but the issue is in sql-bricks) which supports named parameters via a $[name]-syntax. So I tried using sql('$[name]') to use this pattern for example in where-expressions.

But this doesn't work because sql-bricks replaces the "$" with "$1". Now I found out that I can use toString({ placeholder:'?%d' }) which makes everything works, but it feels odd and took me some time to discover. And It's annoying to replace all toString( ) with toString({ placeholder:'?%d' })

Is there a better way to solve this? Maybe with some global config which stops sql-bricks to replace '$' with $i++ altogether? Using autmatic numbering is quite risky anyway.

@prust
Copy link
Collaborator

prust commented Oct 31, 2016

@kay999: Yes, I'm surprised no one has mentioned this before. I prefer named parameters over numbered parameters as well; I'd like to add support, I'll see what impact that they will have on the API and the internals.

It's annoying to replace all toString( ) with toString({ placeholder:'?%d' })

Yes, we should expose the default_opts so that users can change these without needing to specify them each time.

@prust
Copy link
Collaborator

prust commented Oct 31, 2016

@kay999: I wrote a test to demonstrate how I think the API should work with support for named parameters:

it('should supported named placeholders', function() {
  var values = {'first_name': 'Fred', 'last_name': 'Flintstone'};
  var result = insert('user', values).toParams({'placeholder': '$[name]'});

  assert.equal(result.text, 'INSERT INTO "user" (first_name, last_name) VALUES ($[first_name], $[last_name])');
  assert.deepEqual(result.values, {'first_name': 'Fred', 'last_name': 'Flintstone'});
});

However, in trying to implement this I realized that it requires a fairly deep change (using name in the placeholder would trigger "named-placeholder mode", which means all values would be stored in an object instead of an array, and when values are accepted from the user they would need to be supplied in key/value form or the key would need to be implied or possibly auto-generated).

I have a feeling that this kind of change would need to happen as part of a deep refactoring along with some other deep changes aimed at simplifying the implementation (in part by making the inputs less flexible).

In the meantime, does it work to use toString({ placeholder: '' })? That might feel a little less odd, as a way to turn off parameter substitution.

@prust prust mentioned this issue Oct 31, 2016
6 tasks
@kay999
Copy link
Contributor Author

kay999 commented Oct 31, 2016

I created a small pull request (#93) which allows changing the defaults. Works for me.

@kay999
Copy link
Contributor Author

kay999 commented Oct 31, 2016

Ok, thank you, everything works now by using sql.setDefaultOpts({ placeholder:'' }); somewhere.

@prust prust added this to the 3.0 milestone Aug 6, 2017
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

No branches or pull requests

2 participants