-
Notifications
You must be signed in to change notification settings - Fork 45
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
Replace ConnectionError
in public API with io::Error
#135
Replace ConnectionError
in public API with io::Error
#135
Conversation
ConnectionError
in public API with io::Error
8e2a249
to
b4ddd2a
Compare
b4ddd2a
to
fb6d9ab
Compare
@thomaseizinger mind resolving the conflicts here? |
fb6d9ab
to
9e33cd9
Compare
Done! Was just a question of dropping the first commit :) |
9e33cd9
to
cce1012
Compare
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 think my suggestion on libp2p/rust-libp2p#2648 was confusing. I meant to suggest to upstream Into<io::Error> for ConnectionError
to rust-yamux, not to get rid of ConnectionError
on the public interface?
What is the benefit of removing the specific ConnectionError
enum
in favor of the generic io::Error
?
One could argue for a smaller and thus simpler public API but I indeed misunderstood your suggestion :) Happy to also only introduce the mapping function. |
I would argue |
It does! I did also perceive its presence as a bit of an annoyance because we don't recover from any of the enumerated errors. So whilst better use of the type system in theory, it does also create boilerplate. I don't mind either way. |
I think we should assume rust-yamux to not be used by libp2p only. In other words, while libp2p might not make use of it, others might. Also while we don't make use of it today in libp2p, we might at some point. Loosing the details through |
Technically, it is not lost as seen by one of the tests where we recover the inner error, it is just less convenient to access. Will change back to expose |
Closed in favor of #136. |
Draft until #134 is merged.
Extracted out of libp2p/rust-libp2p#2648.