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鈥檒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
Merged

Add withRecursive() #2889

merged 2 commits into from Nov 5, 2018

Conversation

@caseywebdev
Copy link
Contributor

@caseywebdev 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
Author Contributor

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
Author Contributor

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
Copy link
Collaborator

@kibertoad kibertoad commented Nov 4, 2018

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

@caseywebdev
Copy link
Contributor Author

@caseywebdev caseywebdev commented Nov 4, 2018

Comments addressed and docs PR created knex/documentation#144

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 4, 2018

LGTM! @elhigu, what do you think?

@tgriesser tgriesser merged commit c083805 into knex:master Nov 5, 2018
2 checks passed
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
Copy link
Member

@tgriesser 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
Copy link
Contributor Author

@caseywebdev caseywebdev commented Nov 5, 2018

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

Copy link
Member

@elhigu 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
* Add withRecursive()
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.

None yet

4 participants