feat: use source http error instead of application libarary error#5
feat: use source http error instead of application libarary error#5xoac wants to merge 5 commits intoPeggyJV:mainfrom
Conversation
xoac
force-pushed
the
feat/use-source-errors
branch
from
419af83 to
3910c10
Compare
xoac
force-pushed
the
feat/use-source-errors
branch
from
d2fc6f9 to
a2b098f
Compare
xoac
force-pushed
the
feat/use-source-errors
branch
from
a2b098f to
7ecc7b5
Compare
* reqwest, http dependecies update * chore: remove Cargo.lock from lib repo
WalkthroughThis pull request introduces significant updates to the Changes
Sequence DiagramsequenceDiagram
participant Client
participant RegistryCache
participant HTTPClient
participant RemoteRepository
Client->>RegistryCache: get_paths_filtered(tag)
RegistryCache->>HTTPClient: Send request
HTTPClient->>RemoteRepository: Fetch data
RemoteRepository-->>HTTPClient: Return JSON response
HTTPClient-->>RegistryCache: Deserialize response
RegistryCache-->>Client: Return filtered paths
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/cache.rs (1)
Line range hint
74-86: Avoid usingexpectto prevent panics in library codeUsing
expect("path returned None")can cause a panic if the condition is not met. In library code, it's better to handle errors gracefully without panicking.Apply this diff to handle the
Nonecase appropriately:paths.insert( pn.clone(), - get_path(cn[0], cn[1]).await?.expect("path returned None"), + match get_path(cn[0], cn[1]).await? { + Some(path) => path, + None => continue, // or handle the error as needed + }, );
🧹 Nitpick comments (4)
src/get.rs (1)
16-22: Use&strinstead ofStringfor theurlparameterThe
get_typedfunction acceptsurl: String, but it can accept a&str, which avoids unnecessary cloning and can improve performance.Apply this diff to adjust the parameter type:
-async fn get_typed<T: DeserializeOwned>(url: String) -> HttpResult<T> { +async fn get_typed<T: DeserializeOwned>(url: &str) -> HttpResult<T> { let client = reqwest::Client::new(); let req = client - .request(Method::GET, url) + .request(Method::GET, url.to_string()) .header("User-Agent", format!("ocular/{}", VERSION)) .build()?; client.execute(req).await?.error_for_status()?.json().await }Cargo.toml (1)
29-31: Ensure feature flags include all necessary dependenciesThe
reqwest-blockingfeature depends onreqwest-crate/blockingbut doesn't include the basereqwest-cratedependency in its feature list, which could lead to missing features or compilation errors.Apply this diff to include
reqwest-cratein thereqwest-blockingfeature:reqwest = ["reqwest-crate", "reqwest-crate/default-tls"] reqwest-rustls = ["reqwest-crate", "reqwest-crate/rustls-tls"] -reqwest-blocking = ["reqwest-crate/blocking"] +reqwest-blocking = ["reqwest-crate", "reqwest-crate/blocking"]src/cache.rs (2)
Line range hint
32-38: Remove unnecessaryasynckeyword fromget_pathfunctionThe
get_pathfunction does not perform any asynchronous operations but is declared asasync. This adds unnecessary overhead.Apply this diff to remove
async:-pub async fn get_path(&self, chain_a: &str, chain_b: &str) -> HttpResult<Option<IBCPath>> { +pub fn get_path(&self, chain_a: &str, chain_b: &str) -> HttpResult<Option<IBCPath>> {
Line range hint
60-70: Remove unnecessaryasynckeyword fromget_paths_filteredfunctionSince
get_paths_filtereddoesn't perform any asynchronous operations, removingasynccan improve performance.Apply this diff to remove
async:-pub async fn get_paths_filtered(&self, tag: Tag) -> Vec<IBCPath> { +pub fn get_paths_filtered(&self, tag: Tag) -> Vec<IBCPath> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.gitignore(1 hunks)Cargo.toml(1 hunks)src/cache.rs(4 hunks)src/get.rs(8 hunks)tests/registry_cache.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (4)
tests/registry_cache.rs (4)
1-1: Simplify conditional compilation attribute
The change from #![cfg(all(feature = "cache"))] to #![cfg(feature = "cache")] is acceptable and simplifies the condition.
24-26: Remove unnecessary error handling
Since get_paths_filtered now returns Vec<IBCPath> instead of a Result, the error handling can be simplified. The updated code reflects this change appropriately.
31-33: Consistent error handling after signature change
The changes align with the updated return type of get_paths_filtered, removing unnecessary unwrapping of results.
38-40: Maintain consistency in test assertions
The test correctly asserts that the result is not empty and checks the status values.
| use reqwest::Result as HttpResult; | ||
| use reqwest_crate as reqwest; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider encapsulating reqwest errors to avoid exposing dependency details
The use of reqwest::Result (aliased as HttpResult) in public API functions exposes the reqwest crate in your library's public interface. This can cause issues if you switch HTTP clients in the future or if users have version conflicts. It's better to define a custom error type and convert reqwest::Error into your own error before returning it to maintain abstraction.
| async fn get_file_typed_content<T: DeserializeOwned>( | ||
| r#ref: &str, | ||
| path: &str, | ||
| ) -> HttpResult<Option<T>> { | ||
| let url = format!("{}/{}/{}", RAW_FILE_REPO_URL, r#ref, path); | ||
| let response = reqwest::get(url).await?; //.text().await? | ||
|
|
||
| if response.status() == StatusCode::NOT_FOUND { | ||
| return Err(eyre!("path {} not found", path)); | ||
| match reqwest::get(url).await?.error_for_status() { | ||
| Ok(res) => res.json().await, | ||
| Err(e) => { | ||
| if e.status() == Some(StatusCode::NOT_FOUND) { | ||
| Ok(None) | ||
| } else { | ||
| Err(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle network errors separately to ensure robust error handling
In the current implementation of get_file_typed_content, if reqwest::get(url).await? fails due to a network error, the error is propagated immediately, bypassing your error handling logic for HTTP status codes. Refactor the code to separately handle network errors and HTTP errors.
Apply this diff to improve error handling:
- match reqwest::get(url).await?.error_for_status() {
- Ok(res) => res.json().await,
- Err(e) => {
- if e.status() == Some(StatusCode::NOT_FOUND) {
- Ok(None)
- } else {
- Err(e)
- }
- }
- }
+ let response = reqwest::get(url).await;
+ match response {
+ Ok(res) => match res.error_for_status() {
+ Ok(success_res) => success_res.json().await.map(Some),
+ Err(e) => {
+ if e.status() == Some(StatusCode::NOT_FOUND) {
+ Ok(None)
+ } else {
+ Err(e)
+ }
+ }
+ },
+ Err(e) => Err(e),
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn get_file_typed_content<T: DeserializeOwned>( | |
| r#ref: &str, | |
| path: &str, | |
| ) -> HttpResult<Option<T>> { | |
| let url = format!("{}/{}/{}", RAW_FILE_REPO_URL, r#ref, path); | |
| let response = reqwest::get(url).await?; //.text().await? | |
| if response.status() == StatusCode::NOT_FOUND { | |
| return Err(eyre!("path {} not found", path)); | |
| match reqwest::get(url).await?.error_for_status() { | |
| Ok(res) => res.json().await, | |
| Err(e) => { | |
| if e.status() == Some(StatusCode::NOT_FOUND) { | |
| Ok(None) | |
| } else { | |
| Err(e) | |
| } | |
| } | |
| async fn get_file_typed_content<T: DeserializeOwned>( | |
| r#ref: &str, | |
| path: &str, | |
| ) -> HttpResult<Option<T>> { | |
| let url = format!("{}/{}/{}", RAW_FILE_REPO_URL, r#ref, path); | |
| let response = reqwest::get(url).await; | |
| match response { | |
| Ok(res) => match res.error_for_status() { | |
| Ok(success_res) => success_res.json().await.map(Some), | |
| Err(e) => { | |
| if e.status() == Some(StatusCode::NOT_FOUND) { | |
| Ok(None) | |
| } else { | |
| Err(e) | |
| } | |
| } | |
| }, | |
| Err(e) => Err(e), | |
| } |
| eyre = "0.6" | ||
| http = "0.2" | ||
| reqwest = "0.11" | ||
| http = "1" |
There was a problem hiding this comment.
Incorrect version specified for the http crate
The http crate does not have a stable release at version "1". The latest version at the time of writing is "0.2". Specifying an invalid version will lead to dependency resolution errors.
Apply this diff to correct the version:
-http = "1"
+http = "0.2"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| http = "1" | |
| http = "0.2" |
| use reqwest::Result as HttpResult; | ||
| use reqwest_crate as reqwest; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider encapsulating reqwest errors in a custom error type
Exposing reqwest::Result publicly ties your library to the reqwest crate. To maintain abstraction, define a custom error type and convert reqwest::Error into it before returning.
1 participant
According to eyre documentation:
It shouldn't be used in libraries. This PR remove it and instead returns Http Error from request to be handled by application.
Summary by CodeRabbit
New Features
reqwestcrate for enhanced functionality.Bug Fixes
Documentation
Chores
.gitignoreto exclude unnecessary files and directories.