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

Fixes: #2767 Update reqwest-response example #2852

Merged
merged 3 commits into from
Jul 30, 2024
Merged

Conversation

redforks
Copy link
Contributor

Motivation

Update reqwest-response example, as explained by #2767 (comment)

Solution

Based on #2767 (comment)

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

I think there was some sort of confusion in #2767. This example is currently showing how to stream a response from reqwest to a client, it's not showing how to actually proxy a request. The function name proxy_via_reqwest is misleading, but there is a separate example for proxying and I don't think it makes sense to show the same here.

Could you update this PR to only remove the response conversion boilerplate, and leave the request sending as-is? If you want, you can also rename the function to stream_reqwest_response.

reqwest 0.12 updates its http dependency to 1.0, remove two conversion
code:

1. StatuCode
1. Response http headers
@redforks
Copy link
Contributor Author

redforks commented Jul 30, 2024

Reconstruct code:

  1. revert the change that use SyncStream
  2. Remove two unnecessary type conversion: status and headers
  3. Method name renamed

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Thanks, that looks good!

@jplatte jplatte merged commit 8dc371e into tokio-rs:main Jul 30, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants