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

GH-2550 Add support to wrap returning column #2554

Merged
merged 3 commits into from Apr 4, 2018
Merged

GH-2550 Add support to wrap returning column #2554

merged 3 commits into from Apr 4, 2018

Conversation

@kamote
Copy link
Contributor

@kamote kamote commented Apr 2, 2018

Returning clause for oracledb client does not support custom wrapper, this PR add a support for that by calling this.formatter.wrap

  • utilize this.formatter.wrap for every columnName in returningClause
- utilize `this.formatter.wrap` for every columnName in returningClause
@kamote kamote force-pushed the kamote:GH/2550 branch from 414d3f3 to 4120ca7 Apr 2, 2018
Copy link
Member

@elhigu elhigu left a comment

Please describe also which problem/bug this fixes and add tests to make sure there will not be regression (my guess is that current functionality doesn't work for example if custom wrap identifier is used...).

@elhigu
Copy link
Member

@elhigu elhigu commented Apr 2, 2018

Linking related issue #2550 that GH-2550 (which I didn't understand) didn't seem to link correctly between PR and the Issue. So yeah, Only thing missing is the test case for this.

@kamote
Copy link
Contributor Author

@kamote kamote commented Apr 3, 2018

@elhigu Yes you're right, custom wrapper is not implemented/working on returning clause for oracledb client. Please look for the unit test here c2feda5 thanks!

@joelmukuthu

This comment has been minimized.

Copy link
Contributor

@joelmukuthu joelmukuthu commented on test/unit/query/builder.js in c2feda5 Apr 3, 2018

you forgot the .only :)

@kamote
Copy link
Contributor Author

@kamote kamote commented Apr 3, 2018

@joelmukuthu good catch thanks! I've updated the unit test at 904080d

expect(bindings.length).to.equal(6);
expect(bindings[0]).to.equal('foo');
expect(bindings[1]).to.equal('taylor');
expect(bindings[2].toString()).to.equal('[object ReturningHelper:id]');

This comment has been minimized.

@elhigu

elhigu Apr 3, 2018
Member

there must be better way to check these than converting binding to string?

This comment has been minimized.

@kamote

kamote Apr 4, 2018
Author Contributor

This comment has been minimized.

@elhigu

elhigu Apr 4, 2018
Member

ah right... well If that becomes a problem some day it can be fixed then.

@elhigu
elhigu approved these changes Apr 4, 2018
@elhigu elhigu merged commit a85b3a4 into knex:master Apr 4, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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

3 participants