-
Notifications
You must be signed in to change notification settings - Fork 4
Minor change to replace reqwest by ureq #218
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
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 replaces the reqwest HTTP client library with ureq to simplify dependency management and reduce the binary size. The change affects only the BEDbase API client functionality, specifically the method that downloads bedset metadata from the BEDbase API.
Key changes:
- Replaced
reqwestdependency withureqversion 3.1.4 - Updated HTTP GET request implementation in
download_bedset_data()method - Modified error handling to work with ureq's API
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| gtars-bbcache/Cargo.toml | Replaced reqwest dependency (with blocking feature) with ureq 3.1.4 |
| gtars-bbcache/src/client.rs | Updated import statement and HTTP GET implementation to use ureq's API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .body_mut() | ||
| .read_to_string() |
Copilot
AI
Dec 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.
The .body_mut().read_to_string() pattern is incorrect for ureq. The read_to_string() method from std::io::Read requires a mutable String buffer as an argument (e.g., read_to_string(&mut buffer)), but this code doesn't provide one.
For ureq 3.x, the correct pattern is to use .into_string() which consumes the response and returns Result<String, Error>:
let response = get(&bedset_url)
.call()
.map_err(|e| anyhow!("Failed to GET {}: {}", bedset_url, e))?
.into_string()
.map_err(|e| anyhow!("Failed to read response body for {}: {}", bedset_url, e))?;This is more idiomatic, handles errors properly, and avoids the need for the intermediate .body_mut() call.
| .body_mut() | |
| .read_to_string() | |
| .into_string() |
khoroshevskyi
left a comment
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.
Before approving, could you please change request also in core crate for RegionSet ?
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
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
khoroshevskyi
left a comment
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.
Few fixes needed, but everything looks good
For issue 213
Test after replacement