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

Add fetchAsString optional parameter to oracledb dialect #1998

Merged
merged 15 commits into from Apr 28, 2017

Conversation

Projects
None yet
3 participants
@atiertant
Contributor

atiertant commented Apr 4, 2017

add fetchAsString parameter (see https://github.com/oracle/node-oracledb/blob/master/doc/api.md#propdbfetchasstring)

var knex = require('knex')({
  client: 'oracledb',
  connection: {
    host : '127.0.0.1',
    user : 'your_database_user',
    password : 'your_database_password',
    database : 'myapp_test'
  }
  fetchAsString: ['DATE', 'NUMBER', 'CLOB']
});
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 4, 2017

Looks fine except that it should validate input strings and include test and documentation.

testfloat: 7.32,
testdate: new Date(2017, 5, 7)
}).then(function() {
done();

This comment has been minimized.

@elhigu

elhigu Apr 10, 2017

Collaborator

It would be better to just return promise from function instead of calling done in various places.

expect(result[0]).to.be.ok;
expect(result[0].testfloat).to.be.a('string');
expect(result[0].testdate).to.be.a('string');
knexClient.destroy(done);

This comment has been minimized.

@elhigu

elhigu Apr 10, 2017

Collaborator

destroying client here makes all of these tests dependent to each other... sadly thats pretty much how all tests of knex are written.

This comment has been minimized.

@atiertant

atiertant Apr 10, 2017

Contributor

could make two describe creating client in before and destroy in after.

This comment has been minimized.

@elhigu

elhigu Apr 10, 2017

Collaborator

that would be more clean

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 10, 2017

Generally tests and docs looks good. Only thing is that I don't like using done() callback in tests without a good reason, they are more error prone than just returning a promise for test hook.

describe("without fetchAsString parameter", function() {
var knexClient;
before(function(done) {

This comment has been minimized.

@elhigu

elhigu Apr 27, 2017

Collaborator

not necessary done parameter

describe("with fetchAsString parameter", function() {
var knexClient;
before(function(done) {

This comment has been minimized.

@elhigu

elhigu Apr 27, 2017

Collaborator

not necesssary done()

});
describe("OracleDb parameters", function() {
this.timeout(10000);

This comment has been minimized.

@elhigu

elhigu Apr 27, 2017

Collaborator

why this timeout is here?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 27, 2017

Sorry for taking ages with this. I found still couple of minor issues from the code. And probably that timeout should be just removed or use some value that is relative to process.env.KNEX_TEST_TIMEOUT.

@atiertant

This comment has been minimized.

Contributor

atiertant commented Apr 28, 2017

@elhigu timeout was usefull on local only and we removed unnecessary done parameter.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 28, 2017

Now it looks perfect, thanks!

@elhigu elhigu merged commit dd3289b into tgriesser:master Apr 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 28, 2017

I'll try to roll out knex 0.13 during this weekend

@atiertant

This comment has been minimized.

Contributor

atiertant commented Apr 28, 2017

@elhigu that's a good news ! thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment