Skip to content
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

Add RequestExt and RequestPartsExt #1301

Merged
merged 3 commits into from
Aug 22, 2022
Merged

Add RequestExt and RequestPartsExt #1301

merged 3 commits into from
Aug 22, 2022

Conversation

davidpdrsn
Copy link
Member

Fixes #1297

Comment on lines +88 to +94
let mut req = Request::new(());
*req.version_mut() = self.version();
*req.method_mut() = self.method().clone();
*req.uri_mut() = self.uri().clone();
*req.headers_mut() = std::mem::take(self.headers_mut());
*req.extensions_mut() = std::mem::take(self.extensions_mut());
let (mut parts, _) = req.into_parts();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this is kinda unfortunate. As an alternative, we could require B: Default and use mem::take, wdyt about that? Pretty much everything in http-body including the boxed body types seems to implement Default if the inner buffer does, the only exception being Limited but I'm sure there it can be added too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah thats probably fine. I'll change it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm actually its kinda weird having to write this on your extractor. Feels kinda arbitrary from the user perspective 🤔

@@ -180,7 +171,7 @@ mod tests {
     impl<S, B> FromRequest<S, B> for WorksForCustomExtractor
     where
         S: Send + Sync,
-        B: Send + 'static,
+        B: Send + Default + 'static,
         String: FromRef<S> + FromRequest<(), B>,
     {
         type Rejection = <String as FromRequest<(), B>>::Rejection;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I didn't consider that you'd want to use this from functions generic over B. Maybe http can be updated to provide .parts(&self) -> &Parts and .parts_mut(&mut self) -> &mut Parts on Request and Response, the current implementation of the types would trivially allow this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember there was some discussion around that a while ago hyperium/http#511.

@davidpdrsn davidpdrsn merged commit ab36e65 into main Aug 22, 2022
@davidpdrsn davidpdrsn deleted the request-parts-ext branch August 22, 2022 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add helper methods to run easily run extractors
2 participants