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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add withRecursive() #2889

Merged
merged 2 commits into from Nov 5, 2018

Conversation

Projects
None yet
4 participants
@caseywebdev
Copy link
Collaborator

caseywebdev commented Nov 4, 2018

Closes #1755, an alternative to #2218

I needed WITH RECURSIVE in a project recently so I monkey patched this in and it works great 馃憣, figured I'd help out the upstream.

I noticed the other PR tried to handle the UNION ALL automatically for the recursive section, but in my case I actually wanted just a UNION. I think simply having this method add the RECURSIVE keyword is the way to go since it's trivial to do the .union(...) or .unionAll(...) or whatever one needs in the query section of the .withRecursive() method.

Future improvements might include a raw version, but I think getting this added in its simplest form is an easy win.

@caseywebdev caseywebdev force-pushed the caseywebdev:with-recursive branch from f403332 to 48d21f1 Nov 4, 2018

@@ -45,6 +45,22 @@ function Builder(client) {
}
inherits(Builder, EventEmitter);

const validateWithArgs = function(alias, statement, method) {
if (typeof alias !== 'string') {
throw new Error(method + '() first argument must be a string');

This comment has been minimized.

@kibertoad

kibertoad Nov 4, 2018

Collaborator

why not use template string?

This comment has been minimized.

@caseywebdev

caseywebdev Nov 4, 2018

Collaborator

I was simply refactoring this to be used in both with* methods. I've noticed template aren't used in most of this codebase and I assumed this was support older JS interpreters. If you're saying template strings are fine I'll update.

This comment has been minimized.

@kibertoad

kibertoad Nov 4, 2018

Collaborator

Well yeah, support for older JS interpreters was a thing previously, but this year we dropped support for Node.js < 6, so it's good time to start moving towards modern JS syntax :)

},

// Helper for compiling any advanced `withRecursive` queries.
withRecursiveWrapped(alias, query) {

This comment has been minimized.

@kibertoad

kibertoad Nov 4, 2018

Collaborator

why separate this from withRecursive if it just passes same params after validation? Also, should it be public?

This comment has been minimized.

@caseywebdev

caseywebdev Nov 4, 2018

Collaborator

I'm just mirroring the pattern established by with and withWrapped.

while (++i < withs.length) {
const stmt = withs[i];
if (stmt.recursive) recursive = 'recursive ';

This comment has been minimized.

@kibertoad

kibertoad Nov 4, 2018

Collaborator

It's preferable to always use {}

while (++i < withs.length) {
const stmt = withs[i];
if (stmt.recursive) recursive = 'recursive ';

This comment has been minimized.

@kibertoad

kibertoad Nov 4, 2018

Collaborator

This looks a bit redundant, you are resetting string multiple times and constructing string at this time looks kinda premature. Might be cleaner to set boolean flag instead and do something like

const withBlock = isRecursive ? : 'with recursive ' : 'with '

later on

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 4, 2018

Thank you for your contribution! Could you also create a PR in https://github.com/knex/documentation?

@caseywebdev

This comment has been minimized.

Copy link
Collaborator

caseywebdev commented Nov 4, 2018

Comments addressed and docs PR created knex/documentation#144

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 4, 2018

LGTM! @elhigu, what do you think?

@tgriesser tgriesser merged commit c083805 into tgriesser:master Nov 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 84.741%
Details
@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Nov 5, 2018

Yoo @caseywebdev good to see you man! Looks good to me, thanks for the patch!

@caseywebdev caseywebdev deleted the caseywebdev:with-recursive branch Nov 5, 2018

@caseywebdev

This comment has been minimized.

Copy link
Collaborator

caseywebdev commented Nov 5, 2018

Hey Tim! Happy to help 馃憤馃徏 Thanks for the merge 馃巻

@elhigu
Copy link
Collaborator

elhigu left a comment

Only thing to comment from me would be to have integration test making sure that those queries are actually doing what they are meant to do (verifies that written syntax won't get broken even in the future). Looks great though 馃憤

mwilliammyers added a commit to voxjar/knex that referenced this pull request Dec 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment