Rewrite baseTrie as an ES6 class. Extract scratchReadStream to its own file from checkpoint-interface #61
Conversation
alextsg
force-pushed
the
refactor-checkpoint-interface
branch
from
5755b58
to
89d051e
Compare
|
Hi @alextsg, thanks for the short-term update on this. Some first note: even if it might be obvious, can you add reasoning for changes like "extract scratchReadStream to its own file"? This generally avoids misunderstandings / misinterpretations on review or generally overlooking stuff by assuming "ah, this will probably make sense if he does it" and at the end it might not for whatever reason. 😄 Just a procedural thing, haven't had any closer look. |
|
Whew, extensive PR. Seems I won't be able to do an in-tram 🚊 review. 😄 |
|
(btw: of course this is open for review by everyone else too 😄) |
|
@holgerd77 Sure, I'll add that in the PR description, though there's not a huge need for it. I just noticed that the ScratchReadStream class was being defined within checkpoint-interface and thought the code would be more manageable as its own file. Thanks! |
|
Can you eventually do a pre-PR to this one - making sure that documentation is actually building - and then we rebase this on top? I wanted to build documentation locally to get an overview on the exposed API (or at least the documented API 😄) and this seems to be broken atm due to the latest changes. We might generally want to do some changes to the documentation process along with this. Currently this is building to a template file and then this has to be hand-picked to a a final Can you have a look here with the goal of having a non-proxied documentation build of what you think what the current externally exposed API actually is? 😄 If it makes sense this might also be done towards separate documentation files to the different trie types - not sure - haven't put much deeper thought into this, just to make this up as an option. |
|
@holgerd77 Sure I can work on that. And if you want to keep |
|
@holgerd77 What are the requirements for the documentation? Is the handpicking done only to document public methods and remove private methods? If so, I think we can add some things to our JSDoc comments and skip the proxy. |
|
Some meta-note around this: I think this "how is the documentation build" question is some typical example of a case where someone has done something for some reason in some point in time and it is difficult to extract why this is done this way since there is nothing documented around it. I have the impression that I spent far too much time during the year I am working on So if you are confronted with some setup and some current logic and you come to the conclusion that there is a better solution just do the change, since we are doing reviews anyhow I think risk along with this is low (there might be some exception from this rule on security-related changes). I think in this case we can very safely say that a proxy documentation (I actually invented this word, hehe) simply never really makes too much sense. I would say that an API doc should just collect the public methods and skip the rest, not sure why these internal functions are there as well. So yes, we should very much add every API documentation just to the JSDocs, skip the proxy and target a directly build together documentation. |
|
Thanks @holgerd77 . I've created a PR here #63 to address the broken |
|
Cool, thanks! |
|
Did a short try (had a short "is it trie or try" confusion 😛) on rebasing this, but this produces large diffs so I'll leave it to you since you are deeper in the changes. |
|
Sure, will get this done soon. Thanks. |
…n file from checkpoint-interface
alextsg
force-pushed
the
refactor-checkpoint-interface
branch
from
89d051e
to
c2da4c3
Compare
|
Ah, thanks! Always good to drop another note on rebase, since one is not getting any notification on this. Just stumbled over this accidentally cause I am following you on GitHub. 😄 |
|
Just tested, docs is building correctly, looks really nice on first sight. |
| }, cb2) | ||
| /** | ||
| * Retrieves a raw value in the underlying db | ||
| * @method getRaw |
There was a problem hiding this comment.
This is missing the @memberof property and it is therefor not included in the docs (unintended I suppose?).
| * @param {Buffer} value The value to be stored | ||
| * @param {Function} callback A callback `Function`, which is given the argument `err` - for errors that may have occured | ||
| */ | ||
| this.putRaw = this._putRaw |
There was a problem hiding this comment.
This move of putRaw to here puts public API methods into a strange/unordered order in the docs and it is also not so good for code readability to have this proxy method.
Do you have an overview how much effort it would be to remove the proxy method and would it eventually be worth the extra effort? An short-run idea for the documentation order would be to move this JSDoc part down to the _putRaw method (and keep the putRaw naming in the JSDoc comment), a bit cheating but maybe ok in this context.
There was a problem hiding this comment.
Moving the JSDoc part down seems to be a good short term solution. I tried rewriting putRaw as
putRaw (...args) {
return this._putRaw(...args)
}
but ran into RangeError: Maximum call stack size exceeded issues in tests due to checkpoint-interface (some methods seem to be getting swapped there). Once I figure out what's going on there we may be able to remove the proxy method.
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Definitely makes sense to extract this to its own file once one is having a closer look. We can maybe for now also just leave this like you did, so just have it extracted but not (yet?) add this to the official API documentation.
| this._root = value | ||
| }, | ||
| get() { | ||
| return this._root | ||
| } | ||
| }) |
There was a problem hiding this comment.
Constructor is equivalent, ok.
| } else { | ||
| cb2(null, foundNode) | ||
| } | ||
| cb(err, value) | ||
| }) | ||
| } |
| } | ||
| }) | ||
| } | ||
| } |
| } else { | ||
| cb() | ||
| } | ||
| }) | ||
| }) | ||
| } |
|
|
||
| cb(value) | ||
| }) | ||
| } |
| keyEncoding: 'binary', | ||
| valueEncoding: 'binary' | ||
| }, cb) | ||
| async.each(this._putDBs, dbPut, cb) | ||
| } |
| var stack = [] | ||
| targetKey = TrieNode.stringToNibbles(targetKey) | ||
| async.each(this._putDBs, del, cb) | ||
| } |
|
|
||
| stack.push(node) | ||
| async.each(this._putDBs, dbBatch, cb) | ||
| } |
There was a problem hiding this comment.
_putNode(), _batchNodes() equivalent, ok.
| this._lookupNode(root, value => { | ||
| cb(null, !!value) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Remaining functions equivalent as well, done by side-by-side comparison scrolling down editor on left and changes here on the right.
The
ScratchReadStreamclass was being defined withincheckpoint-interface.js. I thought the codebase would be easier to manage if classes were their own separate files, but this is not a necessary change and I'm happy to revert that.