search

datrs / hypercore

Secure, distributed, append-only log
https://docs.rs/hypercore
Apache License 2.0
333 stars 37 forks source link

Upgrade random access storage #17

Closed khernyo closed 6 years ago

khernyo commented 6 years ago

Checklist

Semver Changes

Minor

khernyo commented 6 years ago

This is an incomplete PR, just to show how it would look. A new version is required from all the random-access-* crates to make it work. A couple of small changes are also needed in random-access-memory and random-access-disk, but nothing important.

Is this what you had in mind?

To be honest, I don't think this is the right move. I think I understand why you would want to abstract over the error type, but I would prefer if random-access-storage used the failure crate. The rust community seems to be moving toward using the failure crate for error handling and also integrating parts of it into std. It provides backtrace and a chain of causes, it can be used with no_std and everybody is (or will be) familiar with it. Also, random-access-storage could be used without the <Error = Error> constraint which pretty much forbids any other error types if used with hypercore.

Anyway, if this is how you want to go forward then I'm happy to open the necessary PRs in the random-access-* projects to make this PR work.

yoshuawuyts commented 6 years ago

(...) but I would prefer if random-access-storage used the failure crate (...)

We used to do this, but this prevented us from defining our own error types upstream. E.g. for Error + ErrorKind pattern to work, the RandomAccessStorage trait needs to be generic over the error.

But these changes do look good; I like it!

khernyo commented 6 years ago

Did you mean to push the "Update .github" commit onto this branch or was it by accident? You reverted some of my changes, but only some of them, so it doesn't make sense to me.

The ISSUE_TEMPLATE changes can stay, I don't mind (though I don't think those belong here).

khernyo commented 6 years ago

Could you please release a new random-access-memory and random-access-disk version at the current master? This PR should work with those (after updating the versions in Cargo.toml).

Also, I noticed that you created similar "Update .github" commits in all "random-access-*" repos, and all of them seem to be pushed to old, already merged branches.

yoshuawuyts commented 6 years ago

Also, I noticed that you created similar "Update .github" commits in all "random-access-*" repos, and all of them seem to be pushed to old, already merged branches.

Yeah oops, I ran a batch script but forgot to checkout master first haha. Oops oops.

Did you mean to push the "Update .github" commit onto this branch or was it by accident? You reverted some of my changes, but only some of them, so it doesn't make sense to me.

I did not!! Sorry about that!

yoshuawuyts commented 6 years ago

Published new versions of random-access-disk and random-access-memory!

khernyo commented 6 years ago

Thanks for the releases and no worries about the runaway script :)

It's almost ready to go now, I just need to sign-off on the commit (which I will do later today).

yoshuawuyts commented 6 years ago

@bors r+

yoshuawuyts commented 6 years ago

Okay lol bors isn't going to work now. Merging manually!