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

Conversation

Projects
None yet
2 participants
@schmod
Copy link
Contributor

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

make unionAll()'s call signature match union()
consolidates the logic used by union() and unionAll(), allowing the two to be used interchangeably
@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Feb 26, 2019

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

@schmod

This comment has been minimized.

Copy link
Contributor Author

schmod commented Feb 27, 2019

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

@elhigu

This comment has been minimized.

Copy link
Collaborator

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) {

This comment has been minimized.

Copy link
@elhigu

elhigu Mar 1, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@schmod

schmod Mar 5, 2019

Author Contributor

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.

@elhigu
Copy link
Collaborator

elhigu left a comment

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

@schmod

This comment has been minimized.

Copy link
Contributor Author

schmod commented Mar 5, 2019

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

@elhigu

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Collaborator

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 tgriesser:master Mar 29, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 85.33%
Details

@schmod schmod deleted the schmod:unionAll branch Mar 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.