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
consistently use protocol.ID instead of strings #2004
consistently use protocol.ID instead of strings #2004
Conversation
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.
Can you pull in multiformats/go-multistream#95, so we can see how this will look like in the end?
Should I change network.ConnectionState? |
Do you mean to point the multistream module to the new changes and check? If so, sukunrt/fix-pid-string-conv-ms-merged (https://github.com/sukunrt/go-libp2p/tree/sukunrt/fix-pid-string-conv-ms-merged) |
Nice!!!
I think it would make sense. What do you think? Does it make the code easier? |
I think changing network.ConnectionState is correct. That way internally it is always protocol.ID and we only convert to string when dealing with external lib like tls or translating to protobufs. Updated sukunrt/fix-pid-string-conv-ms-merged with changed network.ConnectionState |
@sukunrt I just cut a new release of go-multistream with all your changes: https://github.com/multiformats/go-multistream/tree/v0.4.0. Can you update this PR to use the release? |
This reduces the string to protocol.ID translations happening at various places in the code
570f5b9
to
d1bceb2
Compare
9e35ce9
to
4ab7c5c
Compare
Updated the branch with all changes merged. |
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 did a full review, and this looks really good to me. One minor thing, then this should be good to merge. 🎉
This reduces the string to protocol.ID translations happening at various places in the code
Fixes #1886