Skip to content

Commit

Permalink
s: tweaks and comments
Browse files Browse the repository at this point in the history
`crates/acap-vapix/src/ajr_http2.rs`:
- Use enum for public error type to get some experience with this
  design even though I think an opaque struct would be more
  appropriate for an error that will change.
- Implement `From<url::ParseError>` is not implemented because I think
  it is less obvious how it should be mapped, so I prefer making it
  explicit to avoid surprises and relieve readers of having to look it
  up.

`crates/acap-vapix/src/http.rs`:
- Use an opaque error type because there are many ways that requests
  can go wrong when crossing a network, and I expect more will be
  explicitly addressed as usage grows. At the same time I don't want
  to expose the underlying `reqwest` errors directly because I want to
  eventually offer support for `curl` and I don't want users to have to
  think about in which underlying library the error originates, and to
  depend on these libraries directly in order to handle errors.
- Use `assert!` instead of `debug_assert!` because most test coverage
  comes from me using the tools that use this library, and and I want
  to continue installing them with the default profile, which afaik is
  the release profile. But I don't think anything terrible will go
  wrong if this assumption does not hold, so long term it is probably
  excessive to crash the program if it does not.
  • Loading branch information
apljungquist committed Aug 26, 2024
1 parent 63100c6 commit e4b9c3f
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 6 deletions.
16 changes: 11 additions & 5 deletions crates/acap-vapix/src/ajr_http2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ use crate::{
http::{HttpError, HttpErrorKind},
};

// Auth, or at least authorization, errors can be communicate using either or both of AJR and HTTP.
// If this and branching on auth errors is common, it may be convenient to lift them out so that
// users don't have to inspect two variants.
// TODO: Consider giving auth errors their own category
#[derive(Debug)]
pub enum AjrHttpError {
// TODO: Consider using something more general to allow request building to fail in other ways.
Expand Down Expand Up @@ -42,6 +46,12 @@ impl Error for AjrHttpError {
}
}

impl From<ajr::Error> for AjrHttpError {
fn from(value: ajr::Error) -> Self {
Self::Procedure(value)
}
}

impl From<HttpError> for AjrHttpError {
fn from(value: HttpError) -> Self {
match value.kind() {
Expand All @@ -56,10 +66,6 @@ impl AjrHttpError {
fn build(e: url::ParseError) -> Self {
Self::Build(e)
}

fn procedure(e: ajr::Error) -> Self {
Self::Procedure(e)
}
}

pub async fn execute_request<S, D>(
Expand Down Expand Up @@ -93,5 +99,5 @@ where
let response_envelope = serde_json::from_str::<ResponseEnvelope<D>>(&text)
.map_err(|e| HttpError::from_status(e, status))?;
debug!("Convert result.");
response_envelope.data().map_err(AjrHttpError::procedure)
Ok(response_envelope.data()?)
}
3 changes: 2 additions & 1 deletion crates/acap-vapix/src/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,8 @@ impl HttpError {
}

fn other_from_reqwest(e: reqwest::Error) -> Self {
debug_assert!(e.status().is_none());
// TODO: Consider demoting to `debug_assert!` or removing this helper entirely.
assert!(e.status().is_none());
Self::other(e)
}

Expand Down

0 comments on commit e4b9c3f

Please sign in to comment.