_lookupNode callback to use standard error, response pattern #83
Conversation
There was a problem hiding this comment.
Thanks for the PR! 👍
This has various linting errors (like e.g. if(e)).
I was puzzled why this has not been catched by linting CI command and did some deeper research. This has actually its root in the .gitignore file having too generic patterns for the specific files from src which are currently build (currently) to the root folder, like baseTrie.js.
This is actually also matching src/baseTrie.js, see this answer on StackOverflow, and was just not discovered since all these files are already added to git and therefore taken along.
All these paths/patterns from baseTrie.js onwards have to be changed in the following way:
baseTrie.js->/baseTrie.js
Then linting will be reactivated. Can you add this here and fix the three additional linting errors which will occur together with the linting errors from your code?
Thanks! 😄 (that was actually a tough one to discover! 😛)
|
@holgerd77 I had forgotten to run the linter. |
src/baseTrie.js
Outdated
| @@ -141,9 +141,11 @@ module.exports = class Trie { | |||
|
|
|||
| if (value) { | |||
| value = new TrieNode(rlp.decode(value)) | |||
| } else { | |||
| err = 'Missing node in DB' | |||
There was a problem hiding this comment.
Can we have this wrapped in an Error object here?
src/baseTrie.js
Outdated
| @@ -375,7 +377,8 @@ module.exports = class Trie { | |||
| return onDone() | |||
| } | |||
|
|
|||
| this._lookupNode(root, node => { | |||
| this._lookupNode(root, (e, node) => { | |||
| if (e) { return onDone(e, node) } | |||
There was a problem hiding this comment.
Can you do this with three-line code style to be consistent e.g. with the code directly below (sorry for the nitpicking 😄)?
src/baseTrie.js
Outdated
| @@ -434,7 +438,8 @@ module.exports = class Trie { | |||
| childKey.push(childIndex) | |||
| const priority = childKey.length | |||
| taskExecutor.execute(priority, taskCallback => { | |||
| self._lookupNode(childRef, childNode => { | |||
| self._lookupNode(childRef, (e, childNode) => { | |||
| if (e) { return cb(e, node) } | |||
|
Thanks for the PR! Just for clarity: how does the behaviour of |
|
Hi Zac, if you feel my 1-vs-3-line if clause comments are too nitty-gritty you have my full understanding and feel free to ignore 😄, but can you address the comment from @s1na? That would be great, then we can merge here, integrate your other fixes and eventually do a release on the library. |
|
Things have gotten busy. I will update this weekend though. And no worries @holgerd77, just wanted to let all the suggestions come in before attempting make another commit. @s1na I dont think the behavior of |
I think it already looks good, and we can merge after you apply @holgerd77 comments. Thanks!
Yeah that's what I meant, I was just trying to say maybe we should add this to the docs, because previously even in this case error would have been |
…ll build files to gitignore
PR #82 broken into 2 pieces
This one doesn't deal with proofs. It just standardizes the callback pattern of
_lookupNode