Use promises instead of callbacks #16
Conversation
bin/hdt
Outdated
| }); | ||
|
|
||
| }, | ||
| (error) => { |
There was a problem hiding this comment.
No parentheses needed (consistency)
lib/HdtDocument.cc
Outdated
| @@ -54,7 +54,7 @@ const Nan::Persistent<Function>& HdtDocument::GetConstructor() { | |||
| Nan::SetPrototypeMethod(constructorTemplate, "_searchTriples", SearchTriples); | |||
| Nan::SetPrototypeMethod(constructorTemplate, "_searchLiterals", SearchLiterals); | |||
| Nan::SetPrototypeMethod(constructorTemplate, "_searchTerms", SearchTerms); | |||
| Nan::SetPrototypeMethod(constructorTemplate, "close", Close); | |||
| Nan::SetPrototypeMethod(constructorTemplate, "_close", Close); | |||
lib/hdt.js
Outdated
| triples, | ||
| totalCount, | ||
| hasExactCount, | ||
| }); |
There was a problem hiding this comment.
No need to return; I'd write the above as
if (err)
reject(err);
else
resolve({ triples, totalCount, hasExactCount })
lib/hdt.js
Outdated
| }, self); | ||
| HdtDocumentPrototype.countTriples = function (subject, predicate, object) { | ||
| return this.search(subject, predicate, object, { offset: 0, limit: 0 }) | ||
| .then(results => ({ totalCount: results.totalCount, hasExactCount: results.hasExactCount })); |
There was a problem hiding this comment.
Maybe remove the then part? Doesn't matter that there's an extra triples field (plus, fewer object shapes for V8).
lib/hdt.js
Outdated
| resolve({ | ||
| literals, | ||
| totalCount, | ||
| }); |
There was a problem hiding this comment.
Idem as above, without return and resolve on a single line.
lib/hdt.js
Outdated
| }; | ||
| HdtDocumentPrototype.close = function () { | ||
| return new Promise((resolve, reject) => { | ||
| this._close(function (e) { |
lib/hdt.js
Outdated
| case 'Non-HDT Section': | ||
| return reject(new Error('The file "' + filename + '" is not a valid HDT file')); | ||
| default: | ||
| reject(error); |
There was a problem hiding this comment.
return missing?
And if so, how did the unit tests miss this?
There was a problem hiding this comment.
A reject handler from a Promise constructor does not need to be returned, it only needs to be called. Added the return here for consistency
There was a problem hiding this comment.
It's not about returning a value, but rather about terminating the function right there. Otherwise, execution continues, and that's not what you want. It's not observable in this particular case though, but we reject because we don't want execution to continue.
test/hdt-test.js
Outdated
| hdt.fromFile(null, function (error) { | ||
| this.should.equal(self); | ||
| it('should throw an error', function () { | ||
| return hdt.fromFile(null).then(() => Promise.reject(new Error('Expected an error')), (error) => { |
There was a problem hiding this comment.
Throughout: no parens around error for consistency.
test/hdt-test.js
Outdated
| }); | ||
| it('should throw an error', function () { | ||
| return document.searchTriples(null, null, null).then( | ||
| () => Promise.reject(new Error('Expected an error')), |
There was a problem hiding this comment.
() => on line above for consistency.
test/hdt-test.js
Outdated
| }); | ||
| it('should throw an error', function () { | ||
| return document.searchLiterals('abc').then( | ||
| () => Promise.reject(new Error('Expected an error')), |
|
fixed the style issues per your latest comments |
|
Thanks for fixing! Given that this will require a new semver major, any other breaking interface changes we might want to consider? Also, README will still need to be updated. |
|
Discussed with @LaurensRietveld that he will add a TypeScript definition for the new interface to this PR. After that, I'll merge and release. |
|
@LaurensRietveld Perfect, thanks! |
|
@RubenVerborgh : would you mind if we merge this to develop? Then we can create a new PR for #17 without having to rebase after. |
|
It's assigned to me, I'll do it today.
|
RubenVerborgh
force-pushed
the
promises
branch
from
b6828e2 to
f39bc59
Compare
|
I just shortened some commit messages to stay below 50 characters, as long commit titles might lead to tooling problems later on. |
RubenVerborgh
force-pushed
the
promises
branch
from
f39bc59 to
3341474
Compare
|
Released as v2.0.0. |
What changed:
triples,totalCount,hasExactCount). As promises should return only one value, I wrapped these in an object.varkeywords toconstif applicableselfreference that were bound to the callbacks. Don't believe there is much need for them anymore with arrow functions, promises, and (on more recent versions of nodejs) async/await.I planned on transforming the
prototypefunctions to a regular es6 class, but that won't do us any good as we shouldnt create our own class, but simply extend the native class by assiging extra functions to its prototype.