-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
Looks fine except that it should validate input strings and include test and documentation. |
test/unit/dialects/oracledb.js
Outdated
testfloat: 7.32, | ||
testdate: new Date(2017, 5, 7) | ||
}).then(function() { | ||
done(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to just return promise from function instead of calling done in various places.
test/unit/dialects/oracledb.js
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
destroying client here makes all of these tests dependent to each other... sadly thats pretty much how all tests of knex are written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could make two describe creating client in before and destroy in after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would be more clean
Generally tests and docs looks good. Only thing is that I don't like using |
test/unit/dialects/oracledb.js
Outdated
describe("without fetchAsString parameter", function() { | ||
var knexClient; | ||
|
||
before(function(done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessary done parameter
test/unit/dialects/oracledb.js
Outdated
describe("with fetchAsString parameter", function() { | ||
var knexClient; | ||
|
||
before(function(done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necesssary done()
test/unit/dialects/oracledb.js
Outdated
}); | ||
|
||
describe("OracleDb parameters", function() { | ||
this.timeout(10000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this timeout is here?
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 |
@elhigu timeout was usefull on local only and we removed unnecessary done parameter. |
Now it looks perfect, thanks! |
I'll try to roll out knex 0.13 during this weekend |
@elhigu that's a good news ! thanks |
add fetchAsString parameter (see https://github.com/oracle/node-oracledb/blob/master/doc/api.md#propdbfetchasstring)