-
Notifications
You must be signed in to change notification settings - Fork 33
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
Support Reqwest Middleware #8
base: main
Are you sure you want to change the base?
Conversation
There are still comments that need to be updated and I may have left some unused imports. I just wanted to know if this is of interest to you before polishing it. |
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.
I can see why this would be valuable.
but I'd rather put this behind a feature flag and make this an alternative via a cfg
, that should be possible
Removed `Box`. Not tested
let mut headers = HeaderMap::new(); | ||
std::mem::swap(&mut headers, resp.headers_mut()); | ||
(resp.status(), resp.url().clone(), headers) | ||
(resp.status(), resp.url().clone(), resp.headers_mut().clone()) |
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.
Was this swap just for performance reasons? It needs to return headers_mut()
response's directly since it will be mutated and isn't just a view "stuck in time".
@@ -47,6 +47,7 @@ impl<T> RequestQueue<T> { | |||
/// Set a delay to be applied between requests | |||
pub fn set_delay(&mut self, mut delay: RequestDelay) -> Option<RequestDelay> { | |||
if let Some((_, d)) = self.delay.as_mut() { | |||
//TODO: take a look. decide if this swap is necessary | |||
std::mem::swap(&mut delay, d); |
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.
My I ask why this swap? Just curious
If that's all I'll make this a feature. |
Co-authored-by: Matthias Seitz <[email protected]>
I suggest abstracting the |
I needed middleware support for a project of mine. My use case is a website that doesn't properly set the content-type charset, so I wanted to set it manually using a reqwest-middleware.
I had to add a
Box
and a.clone()
, this may not be optimal, I'm not that well versed in your code. It's working fine for me. Would this be something you consider including? Maybe behind amiddleware
feature?For reference:
https://github.com/TrueLayer/reqwest-middleware