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

Replace ConnectionError in public API with io::Error #135

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jun 1, 2022

Draft until #134 is merged.

Extracted out of libp2p/rust-libp2p#2648.

@thomaseizinger thomaseizinger changed the title Replace ConnectionError in public API with io::Error Replace ConnectionError in public API with io::Error Jun 1, 2022
@thomaseizinger thomaseizinger force-pushed the refactor/use-io-result-public-api branch from 8e2a249 to b4ddd2a Compare June 1, 2022 18:35
@thomaseizinger thomaseizinger force-pushed the refactor/use-io-result-public-api branch from b4ddd2a to fb6d9ab Compare June 2, 2022 10:57
@mxinden
Copy link
Member

mxinden commented Jun 3, 2022

@thomaseizinger mind resolving the conflicts here?

@thomaseizinger thomaseizinger force-pushed the refactor/use-io-result-public-api branch from fb6d9ab to 9e33cd9 Compare June 3, 2022 15:40
@thomaseizinger thomaseizinger marked this pull request as ready for review June 3, 2022 15:40
@thomaseizinger
Copy link
Contributor Author

@thomaseizinger mind resolving the conflicts here?

Done! Was just a question of dropping the first commit :)

@thomaseizinger thomaseizinger force-pushed the refactor/use-io-result-public-api branch from 9e33cd9 to cce1012 Compare June 3, 2022 15:41
Copy link
Member

@mxinden mxinden left a 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?

@thomaseizinger
Copy link
Contributor Author

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.

@mxinden
Copy link
Member

mxinden commented Jun 8, 2022

One could argue for a smaller and thus simpler public API

I would argue ConnectionError makes better use of Rust's great type system.

@thomaseizinger
Copy link
Contributor Author

One could argue for a smaller and thus simpler public API

I would argue ConnectionError makes better use of Rust's great type system.

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.

@mxinden
Copy link
Member

mxinden commented Jun 8, 2022

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 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 io::Error would be a bummer.

@thomaseizinger
Copy link
Contributor Author

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 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 io::Error would be a bummer.

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 ConnectionError again.

@thomaseizinger
Copy link
Contributor Author

Closed in favor of #136.

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 this pull request may close these issues.

2 participants