-
Notifications
You must be signed in to change notification settings - Fork 96
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
Properly handle LOB types with rowsAsArray set #80
Conversation
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.
Only a few minor comments.
lib/protocol/ResultSet.js
Outdated
p.push(i); | ||
} | ||
return p; | ||
}, []) |
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.
Missing ;
here.
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.
Something simpler like:
var indexes = [];
for (var idx in this.metadata) {
isLob(this.metadata[idx]) && indexes.push(idx);
}
return indexes;
should work as well. This would be simpler, because in your case with reduce
the accumulated value is also an array.
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.
Ok I will change it to a for loop.
@@ -338,6 +338,35 @@ describe('Lib', function () { | |||
}); | |||
|
|||
it( | |||
'should fetch all rows with a LOB and rowsAsArray set', |
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.
Although there are test cases that are not properly formatted, in this file there are still test cases that are properly formatted. Better use proper formatting for your test as well it('...', 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.
I think the formatting is such so that the max line character length in approximately 80.
If you look at the ones that are properly formatted you will notice that they are below 80; however the longer ones that exceed 80 are split into multiple lines.
Fixes #78