-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
Hi @pzagor2, these commit messages are a super-mess 😄, can you please clean them up into one commit with a proper commit message on it? Otherwise thanks for the PR, very appreciated! Cool, that you manually tested the example as well! |
@pzagor2 The easiest way to combine these into one commit is like this:
|
2b34143
to
ade93e5
Compare
Thank you for the comments. I pushed the fixes with cleaned up commit history. |
@pzagor2 Changes look good to me. The Travis build is failing however with a strange error message about a missing ‘./clone.js’ file. I’ll take a look as well... it could just be a bug with Travis |
Ok, I re-ran the travis build and it worked the second time (annoying). @holgerd77 Are you ok with loosening the coverage requirements slightly so this PR passes? The code changes didn't add any new lines of code... the coverage decreased only because the new code was shorter in length. |
@vpulim We already started at some point to add coverage decrease thresholds for the various libraries because that's always some point of pain. I added a decrease threshold of 0.5% now and an absolute threshold of 95%. |
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.
Looks good, see comments from @vpulim.
Ok, just tested, with a review this is mergeable (not sure, maybe this was already be the case before as well, just a red x on the check not necessarily means a block for merging, in the case that the check itself is not set to be mandatory). |
(@vpulim: will leave this open for merge nevertheless, think you are more into the leveldb update topic right now.) |
@holgerd77 Thanks. Merging now. |
This PR updateds
levelup
to the latest version. The need for this update was discussed in this issue forethereumjs-client
project.I ran the tests and they are all passing.
I ran the example code on my Geth DB, it seems to run OK.