Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue: Polish and stabilize the API #267

Closed
2 of 8 tasks
thomaseizinger opened this issue Jan 20, 2023 · 5 comments · Fixed by #272
Closed
2 of 8 tasks

Tracking issue: Polish and stabilize the API #267

thomaseizinger opened this issue Jan 20, 2023 · 5 comments · Fixed by #272

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Jan 20, 2023

The discussion for how we can stabilize the API of this crate takes place here: #259.

This issue is here to track our progress and link to relevant tasks.

Tasks:

  • Split crate into multihash, multihash-codetable and multihash-derive #266
  • Make Error an opaque type
  • Export Multihash as Multihash instead of MultihashGeneric
  • Rename custom-derive to MultihashDigest: Derives are conventionally named after the trait they implement
  • Rename serde-codec feature to serde: Use package renames instead
  • Remove parity-scale-codec dependency? Has a large API surface and is already on major version 3. Doesn't seem to be trust-worthy to not break our API in the future again.
  • Remove write_multihash and read_multihash from the public API: They are already exposed via Multihash::read and Multihash::write
  • Rename arb feature to quickcheck: More conventional

The above list are suggestions. Happy to debate all of them. The thinking was:

  • Reduce and harden API surface
  • Minimize dependencies
  • Follow ecosystem conventions

Overall, my suggestion would be to pack all of these into one breaking change which will hopefully be the last one for a long time.

@vmx
Copy link
Member

vmx commented Jan 20, 2023

  • Rename custom-derive to MultihashDigest: Derives are conventionally named after the trait they implement

The reason why it is named Multihash is that it creates a type called Multihash. So I guess we won't do that anymore and it's just expose the Mulithash<#alloc_size>? Sounds good to me, but will need proper documentation on the upgrade path.

  • Rename serde-codec feature to serde: Use package renames instead

Does this matter much? It sounds to me like a breaking change because we can and not because it's needed. But I don't have a strong opinion, so I'm OK with changing.

  • Remove parity-scale-codec dependency? Has a large API surface and is already on major version 3. Doesn't seem to be trust-worthy to not break our API in the future again.

This seems to be a feature that is actually used, it's contributed by a third part. I've also spotted (indirect) use at https://github.com/ipfs-shipyard/space/pull/15/files#diff-2a805ce01866e6037c0644ed5bb5eb49d4ef87fef1b576f258838ae7c45817a3R12.

  • Rename arb feature to quickcheck: More conventional

The reason it's named arb that it does not only implement quickcheck::Arbitrary but also https://crates.io/crates/arbitrary.

Please make sure you get buy-in from the folks from Forest and Iroh (those are the biggest users that came to my mind), so that we don't end up in a situation where we've forks of multihash again.

@thomaseizinger
Copy link
Contributor Author

  • Rename custom-derive to MultihashDigest: Derives are conventionally named after the trait they implement

The reason why it is named Multihash is that it creates a type called Multihash. So I guess we won't do that anymore and it's just expose the Mulithash<#alloc_size>? Sounds good to me, but will need proper documentation on the upgrade path.

serde_derive::Serialize is called Serialize because it implements serde::Serialize. I find this convention inuitive and would suggest following it too.

I think regular #[deprecation] should work here.

  • Rename serde-codec feature to serde: Use package renames instead

Does this matter much? It sounds to me like a breaking change because we can and not because it's needed. But I don't have a strong opinion, so I'm OK with changing.

Doesn't matter much. We should be able to do it backwards-compatible, i.e. with a deprecation warning first.

  • Remove parity-scale-codec dependency? Has a large API surface and is already on major version 3. Doesn't seem to be trust-worthy to not break our API in the future again.

This seems to be a feature that is actually used, it's contributed by a third part. I've also spotted (indirect) use at https://github.com/ipfs-shipyard/space/pull/15/files#diff-2a805ce01866e6037c0644ed5bb5eb49d4ef87fef1b576f258838ae7c45817a3R12.

I wonder if there is a way for us provide this without it having to be in the library. Does parity-scale-codec offer something like #[serde(with)]? It doesn't need access to internal fields so technically, it should be possible to do this outside of this crate. If we can get a stability commitment I don't think it is a big problem for this to stay.

  • Rename arb feature to quickcheck: More conventional

The reason it's named arb that it does not only implement quickcheck::Arbitrary but also https://crates.io/crates/arbitrary

Didn't see that. Might be worth splitting so folks don't end up with both if they only need one.

Please make sure you get buy-in from the folks from Forest and Iroh (those are the biggest users that came to my mind), so that we don't end up in a situation where we've forks of multihash again.

Definitely. The goal of this is to make it better for everyone!

@thomaseizinger
Copy link
Contributor Author

  • Remove write_multihash and read_multihash from the public API: They are already exposed via Multihash::read and Multihash::write

Turns out this is already sorted because the multihash module is private.

@thomaseizinger
Copy link
Contributor Author

  • Remove parity-scale-codec dependency? Has a large API surface and is already on major version 3. Doesn't seem to be trust-worthy to not break our API in the future again.

This seems to be a feature that is actually used, it's contributed by a third part. I've also spotted (indirect) use at ipfs-shipyard/space#15 (files).

Opened an issue here: paritytech/parity-scale-codec#405

@thomaseizinger
Copy link
Contributor Author

  • Remove parity-scale-codec dependency? Has a large API surface and is already on major version 3. Doesn't seem to be trust-worthy to not break our API in the future again.

This seems to be a feature that is actually used, it's contributed by a third part. I've also spotted (indirect) use at ipfs-shipyard/space#15 (files).

Opened an issue here: paritytech/parity-scale-codec#405

I think for now, we can keep the dependency. We can revisit the idea of removing it once they released another major version. Until then, removing the feature now is just unnecessary friction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants