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
General rework of the library #28
Conversation
This PR is an upstream of the local changes we've been making as part of https://github.com/libp2p/rust-libp2p/tree/master/rust-multiaddr All these changes have already been reviewed. If nobody expresses opposition, I will merge in a few days. |
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.
one comment, other than lgtm
src/protocol.rs
Outdated
|
||
use {Result, Error}; | ||
|
||
///! # Protocol | ||
///! # ProtocolId |
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.
why this rename?
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.
Do you mean this comment in particular, or the whole enum?
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.
I meant the whole enum
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.
That's subjective, but I think ProtocolId
makes it clearer it is not a whole protocol definition for example.
I admit that in practice it's because in my first draft AddrComponent
was named Protocol
.
If you don't like it, I will change it back. I don't have a strong opinion.
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.
lets use Protocol
as it better matches what the go library is doing: https://github.com/multiformats/go-multiaddr/blob/master/protocols.go
Based over #27
Right now if you want to examine the content of the
Multiaddr
, you have to callas_bytes()
and examine bytes one by one and making the assumptions that they are going to be in a certain format.This PR does many minor changes to the library, and most notably adds the
AddrComponent
type which represents one segment of a multiaddr in a strongly-typed fashion. The components can be enumerated withMultiaddr::iter()
. Because of this large change, parsing has also been reworked.