Conversation
This is a breaking change that migrates the HTTP client from synchronous ureq to asynchronous reqwest with Tokio runtime support. Key changes: - Replace ureq with reqwest + tokio in dependencies - Convert all HTTP methods (get, post, put, patch, delete) to async - Update all 23 service modules to use async/await - Update error handling to support reqwest error types - Migrate test infrastructure to use mockito async server - Convert all test functions to async with #[tokio::test] This addresses issues #42 and #76, providing modern async HTTP support for the DNSimple Rust client. BREAKING CHANGE: All API methods are now async and require .await
weppos
added
the
enhancement
Remove unnecessary borrows when passing URLs to reqwest methods.
onlyhavecans
left a comment
onlyhavecans
left a comment
There was a problem hiding this comment.
I have some small style/rust notes and a few blocking things.
in this change we are forcing a change from the rusttls to native-tls (which even reqwest has changed in 0.13, also defaulting to rusttls) so even if we are committing to reqwest 0.12 (which I don't understand why we are doing it in this change) we should also keep "rusttls"
Also choosing to force rt-multi-thread has downstream implementations on people who use our library and we shouldn't force it and let the implementer chose multi or single thread
considering how massively breaking change this is, and how it will require a major release and overhaul by anybody who uses it I highly recommend we also move to edition 2024 and remove all the [#allow(depreciated)] decorators (which will likely be required anyways to update code patterns).
| if response.status().is_success() { | ||
| Self::build_dnsimple_response::<E>(response).await | ||
| } else { | ||
| let body = response.json::<Value>().await.ok(); | ||
| Err(DNSimpleError::parse_response(status, body)) | ||
| } |
There was a problem hiding this comment.
Why is this no longer a match?
| if response.status().is_success() { | ||
| Self::build_empty_dnsimple_response(response).await | ||
| } else { | ||
| let body = response.json::<Value>().await.ok(); | ||
| Err(DNSimpleError::parse_response(status, body)) |
There was a problem hiding this comment.
this still functions as a match
|
|
||
| [dependencies] | ||
| ureq = { version = "2.6", features = ["json"] } | ||
| reqwest = { version = "0.12", features = ["json"] } |
There was a problem hiding this comment.
this is an old version of reqwest? why
| pub fn parse_reqwest_error(error: reqwest::Error) -> DNSimpleError { | ||
| Self::Transport(error.to_string(), "Request error".to_string()) |
There was a problem hiding this comment.
this really isn't a good pattern and we should probably use thiserror better by defining it above in the DNSimpleError like we do all the others.
see https://docs.rs/thiserror/latest/thiserror/
#[error("Request error: {0}")]
Reqwest(#[from] reqwest::Error),| user_agent: String, | ||
| auth_token: String, | ||
| pub _agent: ureq::Agent, | ||
| pub _client: reqwest::Client, |
There was a problem hiding this comment.
Do we intentionally expose this directly? we might want to drop the underscore unless that's intentional
2 participants
Summary
ureqto asynchronousreqwestwith Tokio runtime support#[tokio::test]Closes #42
Closes #76
Breaking Changes
All API methods are now async and require
.await. Example:Dependency Changes
ureqreqwest = "0.12"withjsonfeaturetokio = "1"withrt-multi-threadandmacrosfeaturesTest plan
cargo testruns successfully