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

add With Clause #1599

Merged
merged 6 commits into from Sep 13, 2016
Merged

add With Clause #1599

merged 6 commits into from Sep 13, 2016

Conversation

@atiertant
Copy link
Contributor

@atiertant atiertant commented Jul 28, 2016

fix #716
add a with function(alias, callback|raw)

knex.with('with_alias', function() {
    this.select('*').from('books').where('author', 'Test')
}).select('*').from('with_alias')

output:
with "with_alias" as (select * from "books" where "author" = 'Test') select * from "with_alias"

}).select('*').from('withClause'), {
mssql: 'with [withClause] as (select [foo] from [users]) select * from [withClause]',
sqlite3: 'with "withClause" as (select "foo" from "users") select * from "withClause"',
postgres: 'with "withClause" as (select "foo" from "users") select * from "withClause"',

This comment has been minimized.

@elhigu

elhigu Sep 9, 2016
Member

Is it intentional that many of the tests doesn't have oracle / oracledb result checks?

This comment has been minimized.

@atiertant

atiertant Sep 12, 2016
Author Contributor

in oracle the with clause is used in a select subquery.
this could be used in insert but could not find a way to use it with knex (raw or not...)

if (statement instanceof Raw && arguments.length >= 2) {
return this.withRaw(alias, statement, bindings);
}

This comment has been minimized.

@elhigu

elhigu Sep 9, 2016
Member

I suppose that this line should never be run, since only allowed parameters are function and knex.raw. Throwing exception and adding test for that would be good to prevent undefined does not have property ... errors.

This comment has been minimized.

@atiertant

atiertant Sep 12, 2016
Author Contributor

are you talking about the return in line 83 ?
i could replace return by return this like other functions :
there is no other function who throw any exception in this file...
would you like me to return by:
throw new Error("invalid arguments for with()");

This comment has been minimized.

@elhigu

elhigu Sep 12, 2016
Member

Yep, could be even more detailed describing that argument must be either function / raw. There are no exceptions thrown in this file currently, but actually many of the function does throw an error if called with invalid parameters.

@elhigu
Copy link
Member

@elhigu elhigu commented Sep 9, 2016

Looks good. I added couple of small comments. Also would be good to add at least one integration test for this.

@elhigu elhigu merged commit 69235ae into knex:master Sep 13, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
tgriesser added a commit that referenced this pull request Sep 13, 2016
* master:
  add With Clause (#1599)
tgriesser added a commit that referenced this pull request Sep 14, 2016
* master:
  release 0.12.0
  Remove docs, in favor of https://github.com/knex/documentation (#1666)
  Revert to generic pool (#1665)
  Fix #1619
  Fix use of const in test suite for node 0.12
  Moving bin/cli outside of src to allow install from master
  Deprecate Knex.Promise
  Simplifying internal client structure
  add With Clause (#1599)
  Simplify transaction classes
  Simplify formatter use
  Deprecate VERSION, update changelog
  Fix PG string escaping behavior (#1661)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants