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

Conversation

Projects
None yet
3 participants
@kamote
Contributor

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
GH-2550 Add support to wrap returning column
- utilize `this.formatter.wrap` for every columnName in returningClause
@elhigu

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

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.

Contributor

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

you forgot the .only :)

@kamote

This comment has been minimized.

Contributor

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

Collaborator

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

This comment has been minimized.

@kamote

kamote Apr 4, 2018

Contributor

This comment has been minimized.

@elhigu

elhigu Apr 4, 2018

Collaborator

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 tgriesser:master Apr 4, 2018

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