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

make unionAll()'s call signature match union() #3055

Merged
merged 3 commits into from Mar 29, 2019
Merged

Conversation

@schmod
Copy link
Contributor

@schmod schmod commented Feb 14, 2019

Consolidates the logic used by union() and unionAll(), allowing the two to be used interchangeably (which is the current behavior claimed by the docs).

This allows .unionAll() to be called on multiple subqueries (rather than having to repeatedly call .unionAll() for each one).

Additionally, this adds test-coverage for the undocumented wrap argument on .unionAll() (which is already documented for .union()).


Before:

This was the only allowable call-signature (apart from a second undocumented wrap parameter):

qb.unionAll(subQuery);

If I wanted to write the .unionAll() equivalent of qb.union([sq1, sq2]), I would have needed to write something like:

[s1, sq2].reduce((qb, sq) => qb.unionAll(sq), qb);

After

Any valid call to .union() should now work with .unionAll()

qb.unionAll(sq1);
qb.unionAll(sq1, wrap);
qb.unionAll(sq1, sq2);
qb.unionAll(sq1, sq2, wrap);
qb.unionAll([sq1, sq2]);
qb.unionAll([sq1, sq2], wrap);

Fixes #3054

consolidates the logic used by union() and unionAll(), allowing the two to be used interchangeably
@elhigu
Copy link
Member

@elhigu elhigu commented Feb 26, 2019

Thanks! I’ll check this in near future, with brief look only doc updates are missing...

@schmod
Copy link
Contributor Author

@schmod schmod commented Feb 27, 2019

Got it. I can write a separate PR in the other repo.

@elhigu
Copy link
Member

@elhigu elhigu commented Mar 1, 2019

@schmod thanks, please add a link to that to this PR then, so that it is easy to track when doing release.

wrap: wrap || false,
});
return this;
unionAll(callbacks, wrap) {
Copy link
Member

@elhigu elhigu Mar 1, 2019

It would be more clear to use unionAll(...args) and union(...args) instead of using the old magic arguments array. Now arguments defined for unionAll are not unused.

Copy link
Contributor Author

@schmod schmod Mar 5, 2019

I hesitated to use ...args because knex doesn't use it in many places, and it could potentially create compatibility/performance issues. I can update if we're cool with it though.

Copy link
Member

@elhigu elhigu left a comment

Looks good, only needs some minor fix to method declaration arguments and link to the documentation PR.

@schmod
Copy link
Contributor Author

@schmod schmod commented Mar 5, 2019

Documentation PR: knex/documentation#180
Also updated this PR to use spread args instead of arguments.

@elhigu
Copy link
Member

@elhigu elhigu commented Mar 28, 2019

I think something went really broken in that last change:

for example like this:

 6) Query Building Tests
       QueryBuilder
         multiple union alls:
      mysql: AssertionError: expected 'select * from `users` where `id` = ? union all  union all ' to deeply equal 'select * from `users` where `id` = ? union all select * from `users` where `id` = ? union all select * from `users` where `id` = ?'
      + expected - actual
      -select * from `users` where `id` = ? union all  union all 
      +select * from `users` where `id` = ? union all select * from `users` where `id` = ? union all select * from `users` where `id` = ?

@schmod
Copy link
Contributor Author

@schmod schmod commented Mar 29, 2019

@elhigu My bad. Just pushed a change that should have fixed it.

I'm still seeing a failure on the Node 8 and Oracle tests, but it looks like those are unrelated to this PR.

elhigu
elhigu approved these changes Mar 29, 2019
@elhigu
Copy link
Member

@elhigu elhigu commented Mar 29, 2019

I restarted node 8 tests, since it looked like random fail.... will merge after those pass. Thanks!

@elhigu elhigu merged commit b15ee3d into knex:master Mar 29, 2019
2 checks passed
@schmod schmod deleted the unionAll branch Mar 29, 2019
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

2 participants