-
Notifications
You must be signed in to change notification settings - Fork 4
Merge async client #8
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces an asynchronous client module to support non-blocking HTTP requests for the Sanity client and updates the Cargo.toml accordingly.
- Added a new async_client module with configuration and helper functions
- Introduced asynchronous implementations for fetching and processing JSON data
- Updated versioning and added the tokio dependency
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Added declaration for the new async_client module |
| src/async_client/mod.rs | Created module structure and re-exporting of config and helpers |
| src/async_client/helpers.rs | Added async helper to process JSON responses from reqwest |
| src/async_client/config.rs | Added configuration, URL building, and async query execution functions |
| Cargo.toml | Updated package version and added the tokio dependency |
|
|
||
| impl Query { | ||
| pub async fn execute(&self) -> Result<Value, Box<dyn std::error::Error>> { | ||
| let url = format!("{} {}", &self.base_url, &self.query.as_ref().unwrap()); |
Copilot
AI
May 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concatenating the base URL and query with a space may lead to an improperly formatted URL. Consider using proper query parameter formatting (e.g., using '?' or '&' as appropriate) and ensuring the query is correctly appended.
| pub async fn get_json( | ||
| reqwest_res: reqwest::Response, | ||
| ) -> Result<Value, Box<dyn std::error::Error>> { | ||
| let data: Value = serde_json::from_str(&reqwest_res.text().await?)?; |
Copilot
AI
May 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Instead of converting the response text to JSON manually, it might be clearer and more efficient to use reqwest's built-in JSON deserialization with reqwest_res.json().await.
| let data: Value = serde_json::from_str(&reqwest_res.text().await?)?; | |
| let data: Value = reqwest_res.json().await?; |
@gibsorya did some excellent work, merging in to upstream :)