Comments (14)
Code
uses conditional enum variants. That is not ideal. Features are additive across a dependency tree in Rust. Conditional enum variants can turn an exhaustive match in a library into a non-exhaustive one simply because the end-user activates more features ofmultihash
.
Code was introduced for backwards compatibility and kind of an "easy to get started" way. Back then, without out it, rust-libp2p wouldn't have upgraded to rust-multihash, but kept their fork. Power users should always define their own code table. So perhaps moving into its own crate makes sense, so people that don't want to define their own table can just use that.
from rust-multihash.
Code
uses conditional enum variants. That is not ideal. Features are additive across a dependency tree in Rust. Conditional enum variants can turn an exhaustive match in a library into a non-exhaustive one simply because the end-user activates more features ofmultihash
.Code was introduced for backwards compatibility and kind of an "easy to get started" way. Back then, without out it, rust-libp2p wouldn't have upgraded to rust-multihash, but kept their fork. Power users should always define their own code table. So perhaps moving into its own crate makes sense, so people that don't want to define their own table can just use that.
Thanks for sharing that bit of history! I wasn't aware of that. I am in favor of creating a standard-multihashes
crate or something like that where all of this goes. (It should likely still not use conditional enum variants for the reasons stated above.)
from rust-multihash.
I'd like to add that the idea that people would use a custom code table and not use the bundled conditional enum clearly failed. So what if we move that part into a separate crate without any features. It will just bundle some common ones (probably not all of the current ones). It would tick the box of "easy to get started" and if people that don't want to draw in hash functions they don't need, they can create their own table.
from rust-multihash.
Yeah that sounds pretty reasonable to me! :)
Would you move the custom-derive too? Something like:
multihash-codetable
multihash-codetable-derive
from rust-multihash.
Would you move the custom-derive too?
It's already its own crate, so I would just keep its current name.
from rust-multihash.
Would you move the custom-derive too?
It's already its own crate, so I would just keep its current name.
Good point. By convention however, I'd expect the trait that the derive macro implements lives in the "sister"-crate, i.e. multihash-derive
should derive a trait that lives in multihash
. The trait that is being implement is however not necessary for the core Multihash
datastructure.
We could fix this by extracting a single multihash-core
crate that only has that data structure in it and leave the multihash
crate otherwise as is.
I don't really mind either way but I think it would be good to honor these conventions so either:
- Extract
multihash-core
- Create two new crates
multihash-codetable
andmultihash-codetable-derive
(All names subject to bike-shedding.)
from rust-multihash.
I would keep crate renames to a minimum, so that downstream users have as little work as possible.
from rust-multihash.
For users of multihash
, we should be able to ship this as a non-breaking change by re-exporting doing pub use multihash_core::Multihash
. Crates like multiaddr
will then have manually change their dependency to only depend on multihash-core
.
from rust-multihash.
Personally, I'd prefer to pick alternative names. The name multihash-core
doesn't mean much and it feels wrong to pay the cost of a less expressive name for the time coming vs spending a one-time effort now to fix it.
I think we can ship a transition to my proposed structure in a backwards-compatible way, such that users don't need to rename their crates right away but only once they want to fix the deprecation warnings. In general, switching to a different crate with the same API is really minimal effort.
- Create
multihash-codetable
crate by extracting theCode
enum etc frommultihash
- Remove
Code
frommultihash
and publish as a new breaking change. - Create a patch-release for the version before the breaking change where we depend on the new minor version and re-export
multihash
. This is utilizing https://github.com/dtolnay/semver-trick. - Within the same patch version, mark all items as deprecated in
multihash
, telling users to depend onmultihash-codetable
instead of they want to use these features. multihash-codetable
can depend on the new minor version ofmultihash
.
We should be able to do this because we are not actually changing any APIs in a backwards incompatible way but just need to align the types properly.
from rust-multihash.
The process looks good. Though I would keep names. I would name what you call multihash-core
simply multihash
and name multihash-codetable-derive
-> multihash-derive
. The only change would be a new crate called multihash-codetable
. This way we don't end up with deprecated crates for the sake of it. Though I don't have a really strong opinion about it, so input from others would be appreciated.
from rust-multihash.
The process looks good. Though I would keep names. I would name what you call
multihash-core
simplymultihash
I think we are on the same page here. multihash-core
was only a proposal if we wanted multihash
to remain the crate with the code table in it. I prefer to strip multihash
from the code table and only have to Multihash
data structure in it.
and name
multihash-codetable-derive
->multihash-derive
. The only change would be a new crate calledmultihash-codetable
. This way we don't end up with deprecated crates for the sake of it. Though I don't have a really strong opinion about it, so input from others would be appreciated.
I can definitely see the benefit in not abandoning crate names so I am happy to go that way.
Final question: Which crate do you see the MultihashDigest
trait in?
- If we keep it in
multihash
, then the naming of themultihash-derive
crate follows conventions in the ecosystem (serde
->serde-derive
, f.e.). - If the trait moves into
multihash-codetable
then the naming is a bit weird but I could get around to it.
Initially I thought that the trait would move to multihash-codetable
because strictly speaking, it is not necessary for the Multihash
data structure. However, now that I think about it, I think it would actually be better if that trait stays in the multihash
crate. It will allow users to be abstract over code tables which seems like a nice property.
from rust-multihash.
As I also propose to remove the feature flags from multihash-codetable
(though that's open for discussion), it would be a "omg, it pulls in all those dependencies" crate. But that would exactly what I'm after. For toy projects, it doesn't matter, for bigger projects, define your own table.
This means that the multihash-codetable
should only be used for the code table and nothing else. Hence I'm for keeping MultihashDigest
in the multihash
crate.
from rust-multihash.
As I also propose to remove the feature flags from
multihash-codetable
(though that's open for discussion), it would be a "omg, it pulls in all those dependencies" crate. But that would exactly what I'm after. For toy projects, it doesn't matter, for bigger projects, define your own table.
Sounds good to me.
This means that the
multihash-codetable
should only be used for the code table and nothing else. Hence I'm for keepingMultihashDigest
in themultihash
crate.
Sweet, I'll try to make some time for this tomorrow and prepare some PRs / branches.
from rust-multihash.
I don't think there is anything particularly actionable any more in here.
from rust-multihash.
Related Issues (20)
- Implement ripemd160 HOT 3
- CI tries to run `fmt` with unstable feature and will always fail HOT 2
- Nix build is currently broken HOT 3
- Maintain a CHANGELOG.md HOT 3
- Replace or Upgrade Tarpaulin Code Coverage HOT 2
- Split crate into `multihash`, `multihash-codetable` and `multihash-derive` HOT 5
- Tracking issue: Polish and stabilize the API HOT 5
- Use `sha1` dependency instead of `sha-1`
- Unsafe unwrap could cause an application to panic HOT 12
- Consider removing `MultihashDigest` and `Hasher` traits
- Write docs for transition to new crate structure HOT 6
- Parity Codec bumps MSRV to `1.64.0` HOT 4
- Add `cargo semver-checks` to CI to ensure we don't break the API accidentially HOT 1
- Add Sha3 SHAKE* HOT 12
- Only enforce MSRV if no features are activated
- Release 0.19 HOT 8
- multihash const constructors unusable HOT 5
- Redesign multihash::Error
- regression serde feature does not work with no-std
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from rust-multihash.