Convert trieNode to ES6 class #71
Conversation
Signed-off-by: Sina Mahmoodi <[email protected]>
Signed-off-by: Sina Mahmoodi <[email protected]>
Signed-off-by: Sina Mahmoodi <[email protected]>
Signed-off-by: Sina Mahmoodi <[email protected]>
Signed-off-by: Sina Mahmoodi <[email protected]>
Signed-off-by: Sina Mahmoodi <[email protected]>
Signed-off-by: Sina Mahmoodi <[email protected]>
|
Can we name this |
|
Update: just saw that there was a file |
|
@holgerd77 I agree the name is a bit confusing (can be confused with I'll try to make this more clear during typescript transition by using types. Update: I'll be happy to change the name if you have suggestions? (other than utils, I really dislike anything called utils 😄) |
|
One first-thought suggestion: we could add a directory |
| @@ -5,7 +5,7 @@ | |||
| "main": "index.js", | |||
| "scripts": { | |||
| "test": "npm run test:node && npm run test:browser", | |||
| "coverage": "istanbul cover tape ./test/*.js", | |||
| "coverage": "istanbul cover babel-tape-runner ./test/*.js", | |||
There was a problem hiding this comment.
I am not understanding the intention respectively the effect of this change, could you describe?
There was a problem hiding this comment.
The coverage tests were failing due to some ES6 syntax. Unit tests were already using babel-tape-runner ("test:node": "babel-tape-runner ./test/*.js",).
src/hex-prefix.js
Outdated
| export function doKeysMatch (keyA, keyB) { | ||
| var length = matchingNibbleLength(keyA, keyB) | ||
| return length === keyA.length && length === keyB.length | ||
| } |
There was a problem hiding this comment.
You don't have to go through the whole library on this, but since we are aggregating here could you have at least for the functions included in this file have some look at the JSDoc API comments? These are very inconsistent with missing values and stuff.
| this.setKey(key) | ||
| } | ||
| } | ||
| } |
src/hex-prefix.js
Outdated
| } | ||
|
|
||
| /** | ||
| * Determines if a key has Arnold Schwarzenegger in it. |
There was a problem hiding this comment.
I find this kind-of funny but this is nevertheless irritating and I already "stumbled" over this several times and it prevents getting what this really does. 😄 Could you replace with a proper description of the functionality provided?
There was a problem hiding this comment.
Haha, yeah I had the same feeling when I saw this :)) Added a description.
| } else { | ||
| key = key.slice(0) // copy the key | ||
| if (arguments.length === 1) { |
There was a problem hiding this comment.
Phew, this kind of stuff is really very ugly and hard to read (was puzzled for some time regarding the function call forwardings of the respective get and set methods. We should keep it for now but also change this mid-term.
| } | ||
|
|
||
| return 'extention' | ||
| return children |
There was a problem hiding this comment.
Ok, I've gone through the trieNode.js file function by function and compared, seems to look good.
Signed-off-by: Sina Mahmoodi <[email protected]>
Signed-off-by: Sina Mahmoodi <[email protected]>
This PR converts
TrieNodeto an ES6 class and extracts hex prefix encoding functions to a different file, as hex prefix is an encoding method and is conceptually separate from a trie node (although encoding is used in the trie node class). It also transforms source files withbabelifyfor karma tests.If I'm not mistaken, hex prefix functions were not exposed and therefore this shouldn't be an API change.