Go off the rails #32
Go off the rails #32
Conversation
seffradev
left a comment
seffradev
left a comment
There was a problem hiding this comment.
Just leaving a partial review because I am having a lazy Saturday :)
arirs/src/request_client/mod.rs
Outdated
| pub(crate) async fn authorized_get<T: Serialize, R: DeserializeOwned>(&self, path: impl AsRef<[&str]>, params: T) -> Result<R> { | ||
| let url = self.authorized_url(path, params)?; | ||
| let response = self.as_ref().get(url).send().await?.json().await?; | ||
| Ok(response) | ||
| } | ||
|
|
||
| pub(crate) async fn authorized_post<T: Serialize>(&self, path: impl AsRef<[&str]>, params: T) -> Result<()> { | ||
| let url = self.authorized_url(path, params)?; | ||
| self.as_ref().post(url).send().await?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub(crate) async fn authorized_post_json_response<T: Serialize, R: DeserializeOwned>( | ||
| &self, | ||
| path: impl AsRef<[&str]>, | ||
| params: T, | ||
| ) -> Result<R> { | ||
| let url = self.authorized_url(path, params)?; | ||
| let response = self.as_ref().post(url).send().await?.json().await?; | ||
| Ok(response) | ||
| } | ||
|
|
||
| pub(crate) async fn authorized_post_variables<T: Serialize, R: DeserializeOwned>( | ||
| &self, | ||
| path: impl AsRef<[&str]>, | ||
| params: T, | ||
| variables: &HashMap<&str, &str>, | ||
| ) -> Result<R> { | ||
| let url = self.authorized_url(path, params)?; | ||
| let response = self | ||
| .as_ref() | ||
| .post(url) | ||
| .json(&serde_json::json!({ | ||
| "variables": variables | ||
| })) | ||
| .send() | ||
| .await? | ||
| .json() | ||
| .await?; | ||
| Ok(response) | ||
| } | ||
|
|
||
| pub(crate) async fn authorized_delete<T: Serialize>(&self, path: impl AsRef<[&str]>, params: T) -> Result<()> { | ||
| let url = self.authorized_url(path, params)?; | ||
| self.as_ref().delete(url).send().await?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn authorized_url<'a, T: Serialize>(&self, path: impl AsRef<[&'a str]>, params: T) -> Result<Url> { | ||
| let mut url = self.url().join(&path.as_ref().join("/"))?; | ||
| self.set_authorized_query_params(&mut url, params); | ||
| Ok(url) | ||
| } | ||
|
|
||
| pub(crate) fn authorize_request<T>(&self, inner: T) -> AuthorizedRequest<T> { | ||
| AuthorizedRequest { | ||
| api_key: self.get_api_key(), | ||
| inner, | ||
| } | ||
| } |
There was a problem hiding this comment.
question: why are all these named something with authorize? I may be wrong, but I don't know of any unauthenticated operations in ARI.
There was a problem hiding this comment.
fair :) I'll take a look at this once I've gained some more experience for working with the lib and ARI
arirs/src/channel.rs
Outdated
| #[instrument(level = "debug")] | ||
| pub fn start_moh(&self, _client: &Client) -> Result<()> { | ||
| unimplemented!() | ||
| impl RequestClient { |
There was a problem hiding this comment.
suggestion: if we're doing a refactor of how the different modules work, can't we abstract out the different model functionalities (bridge, channel, playback) to be implemented traits so we don't populate RequestClients internal namespace?
There was a problem hiding this comment.
Possibly, I was thinking about simply prefixing the methods with playback_, bridge_ etc.
There was a problem hiding this comment.
I still think that bloats the namespace a bit too much. It does improve the scope of functions but I generally liked the previous interface of Channel::func(&client) so I think a use ChannelExt; client.originate(params); would be cleaner. I also think this would improve the ability to e.g. mock which could be nice for tests. Though mocks are generally bad :)
There was a problem hiding this comment.
I've also thought about implementing traits on the client. Problem is that a lot of functions already collide without a prefix (bridge/channel, live/stored recording, listing and getting by id). Prefixes will in many cases be needed anyways, or there will be quite a few cases where disambiguation is required, ex. <RequestClient as ChannelClient>::stop_moh(&client, params), which I think should be avoided as much as possible.
There's probably also plenty of work that can be done do reduce the number of ResponseClient methods in the first place. Say by ombining the _with_id() methods to their counterparts by setting the id optional. Or say by using generics on the common create, list, get, remove/destroy metods. I think that this should be the primary focus point. The type system should be able to infer a whole lot if we typeset the ids and implement some trait with associated consts and types on them.
My fault that this is 40 commits long 😅 |
gibbz00
force-pushed
the
off_the_rails
branch
from
2c10d95 to
51dafe2
Compare
Libraries should avoid locking in logging solutions. Future work could instead instrument the function behind a feature flag.
- Place `Debug` first - Request params derives `Serialize` and `Debug` only. Fields are all correctly serialized as "camelCase". (Rename all is sometimes superflously applied to avoid future compatibility issues.) - Response data derives `Deserialize` only. Their fields are made private, exposed through `Getters` derive.
Also removes `tracing` library lock-in.
gibbz00
force-pushed
the
off_the_rails
branch
from
51dafe2 to
ddc3b3d
Compare
Begin moving away from crate-wide error.
|
Satisfied now :) |
2 participants
No description required.
Depends on #31