multiformats / rust-multiaddr

multiaddr implementation in rust
https://crates.io/crates/multiaddr
Other
87 stars 46 forks source link

Use `impl From` instead of `trait ToMultiaddr` #23

Closed crackcomm closed 3 years ago

crackcomm commented 7 years ago

It seems to me that there is too much of to_* and from_* when it should be enough to use .into() and single impl From.

dignifiedquire commented 7 years ago

Yes makes more sense probably. Though I am not sure, is there any case were you shouldn't use impl From or is it okay to use for all conversions?

crackcomm commented 7 years ago

Usage of impl From should be limited to use cases that are conversions between types.

Traits like ToString or FromStr should be implemented in cases of deserialization/serialization when input can be invalid but output is always possible.

In this case it seems perfectly fine and more valid to have:

impl From<Ipv4Addr> for Multiaddr

instead of

impl ToMultiaddr for Ipv4Addr

because as such it is only a type conversion.

dignifiedquire commented 7 years ago

Thanks for the explanation.

tomaka commented 6 years ago

Note that this should be TryFrom and not just From, because the conversion can return an error.

jcc333 commented 6 years ago

So I'd love to contribute to this project with the caveat that I'm coming from a c++/scala/haskell background and trying to learn rust so there might be a vocabulary gap. That said I have a couple of questions around the context of this issue:

mxinden commented 3 years ago

Closing here since this is addressed with https://github.com/multiformats/rust-multiaddr/pull/40.

@jcc333 let us know in case you would still like to contribute.