-
Notifications
You must be signed in to change notification settings - Fork 149
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
feat(api-client-framework)!: configuration and endpoint updates #1174
Conversation
…t reqwest related crates
…for client config; add headers setup into Endpoint trait
WalkthroughThe pull request introduces modifications to the API client framework in Rust. The changes primarily focus on refactoring the HTTP client configuration and endpoint handling. The Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (5)
libs/api-client-framework/src/lib.rs (1)
9-11
: Heads up on re-exported crates
Re-exportingreqwest
,reqwest_middleware
, andurl
directly from this library may lead to version conflicts in downstream consumers if they already depend on different versions of these crates. Consider exposing only the necessary types or functions to mitigate potential collisions while preserving modularity.libs/api-client-framework/src/endpoint.rs (1)
26-32
: Consider providing additional guidance for setting custom headers.
The newheaders()
method provides a clean way to specify optional request headers. If certain headers must always be present (for example, authentication tokens or correlation IDs), consider documenting or enforcing this in your implementing types. Otherwise, this looks good and aligns well with the trait design.libs/api-client-framework/src/async_client.rs (3)
15-16
: Provide examples or references on how to implement custom middlewares.
Declaringmiddlewares: Vec<Arc<dyn Middleware>>
is an excellent way to allow layered request processing. For clarity, adding an inline example or referencing a “How to implement custom middlewares” doc might help maintainers and contributors.
41-46
: Order-based application of middlewares is correct.
Applying each middleware in the sequence they appear inconfig.middlewares
is straightforward. If you rely on specific ordering, consider documenting it in the config struct docstring. Otherwise, this looks good.
71-74
: Be mindful of overwriting any previously set headers.
Callingrequest.headers(...)
replaces the entire header map. If you need to preserve headers added earlier (likeContent-Type
), consider merging them instead. Otherwise, this approach correctly applies endpoint-specific headers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/api-client-framework/Cargo.toml
(0 hunks)libs/api-client-framework/src/async_client.rs
(3 hunks)libs/api-client-framework/src/endpoint.rs
(1 hunks)libs/api-client-framework/src/lib.rs
(1 hunks)
💤 Files with no reviewable changes (1)
- libs/api-client-framework/Cargo.toml
🔇 Additional comments (2)
libs/api-client-framework/src/async_client.rs (2)
4-6
: No issues found with the new imports.
This import ofClientBuilder
andMiddleware
is consistent with the updated approach that removes explicit retry logic.
24-24
: Empty default is reasonable.
Initializingmiddlewares
as an empty vector aligns with the typical usage pattern.
CustomError
from Error enummax_retries
variableEndpoint
traitSummary by CodeRabbit
New Features
Refactor
Chores
reqwest-retry
dependency from project configuration