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

Use error enums instead of Anyhow #106

Closed
15 tasks done
Tracked by #1
algesten opened this issue Oct 5, 2021 · 7 comments · Fixed by #109
Closed
15 tasks done
Tracked by #1

Use error enums instead of Anyhow #106

algesten opened this issue Oct 5, 2021 · 7 comments · Fixed by #109

Comments

@algesten
Copy link
Member

algesten commented Oct 5, 2021

Original discussion on discord, and further issue raised here: webrtc-rs/rtp#14

Anyhow is great, but maybe not a good fit for webrtc.rs. To summarize:

  • Anyhow is mostly meant for applications, not libraries.
  • Anyhow doesn't implement std::error::Error (it can't), which makes it hard to interoperate in apps that rely on that.
  • Not having explicit enumerations of possible errors means the webrtc.rs API effectively have "hidden" code paths. It's not possible for a user to know which errors could potentially be thrown from an API call returning anyhow::Result<X>

Way forward

  • Replace anyhow with idiomatic Rust error enums.
  • Keep thiserror to help implement said enums.
  • Maintain ergonomics for ? use internally in webrtc.rs (work with From traits and rewrap errors).
  • Bring out possible errors in API calls. Either via types or documentation.

This issue will be used to coordinate the effort which will span all the sub-crates.

  • sdp
  • util (done, but deliberately marked as draft)
  • mdns
  • stun
  • turn
  • ice
  • dtls
  • rtp
  • rtcp
  • srtp
  • sctp
  • media
  • interceptor
  • data
  • webrtc
@algesten
Copy link
Member Author

algesten commented Oct 5, 2021

First stab at sdp is done. Next up I'll do util. I want to do from the "bottom up" following the dependency tree, and we won't get a good idea of how this PR will work before we get to the second/third level of dependencies.

@algesten
Copy link
Member Author

algesten commented Oct 5, 2021

@rainliu Would you mind looking at the two first PRs? I'm wondering what you think about it so far. I've hard to maintain the convenience of ? and PartialEq.

@rainliu
Copy link
Member

rainliu commented Oct 6, 2021

@algesten, thanks for the efforts.

PR for sdp crate looks good to me.

and I leave some comments on PR for util crate.

@algesten
Copy link
Member Author

algesten commented Oct 6, 2021

Alright. That's all for me for today. I'll continue tomorrow.

@algesten
Copy link
Member Author

algesten commented Oct 7, 2021

@rainliu I believe this is done. The next step is probably reviewing, merging and releasing new versions of the subcrates.

@algesten
Copy link
Member Author

algesten commented Oct 8, 2021

@rainliu I've fixed clippy lints in all the PRs, and also speculatively bump deps everywhere. I assumed since this is a breaking change, we're doing minor revisions everywhere. This is the version plan I've bumped to:

sdp 0.3.0 
util 0.5.0 (released already)
mdns 0.4.0
stun 0.4.0
turn 0.5.0
ice 0.5.0
dtls 0.5.0
rtp 0.5.0
rtcp 0.5.0
srtp 0.6.0
sctp 0.4.0
media 0.3.0
interceptor 0.3.0
data 0.3.0

@algesten
Copy link
Member Author

algesten commented Oct 8, 2021

The idea is that if you merge/bump (to the version above)/release in the right order, it should "just work"

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 a pull request may close this issue.

2 participants