-
Notifications
You must be signed in to change notification settings - Fork 20
Conversation
License: MIT Signed-off-by: Jacob Heun <jacobheun@gmail.com>
0e09be4
to
75dd480
Compare
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.
LGTM
src/errors.js
Outdated
module.exports.ERR_DB_CANNOT_OPEN = (err) => { | ||
err = err || new Error('Cannot open database') | ||
return errcode(err, 'ERR_CANNOT_OPEN_DB') | ||
} |
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.
ERR_DB_OPEN_FAILED
for consistency with the others?
src/errors.js
Outdated
module.exports.ERR_NOT_FOUND = (err) => { | ||
err = err || new Error('Not Found') | ||
return errcode(err, 'ERR_NOT_FOUND') | ||
} |
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.
Would it be better for these factory functions to be named more like dbDeleteError()
, notFoundError()
? All caps var names are by popular convention constant values and not functions.
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.
Updated the function names and the open failure code
License: MIT Signed-off-by: Jacob Heun <jacobheun@gmail.com>
@dignifiedquire it looks like you and @diasdavid are the only ones with publish permissions to npm for this repo. With him out until Friday would you be able to do a release of this so we can get js-ipfs#1557 resolved? |
I also updated the dependencies to their latest versions.
I have also run the updated tests locally against the error implementation in datastore-fs for reference. https://github.com/ipfs/js-datastore-fs/compare/feat/err-codes?expand=1 (note: the ci tests will fail there until the dependency is updated)