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

Allow returning memoryview in StreamingResponse #2576

Merged
merged 1 commit into from Apr 22, 2024

Conversation

bschoenmaeckers
Copy link
Contributor

@bschoenmaeckers bschoenmaeckers commented Apr 22, 2024

Summary

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Makes sense to me!

@adriangb adriangb merged commit 9fd3ecc into encode:master Apr 22, 2024
5 checks passed
@Kludex
Copy link
Sponsor Member

Kludex commented Apr 23, 2024

What's the use case? To create the memoryview you need bytes or bytearray anyway...

@bschoenmaeckers
Copy link
Contributor Author

Memoryview allows you to efficiently return a slice of the original bytes value without copying. This is especially convenient when you want to strip some prefix or header of some value in memory you don’t control.

@adriangb
Copy link
Member

Yup I'd say the use cases are varied, it allows moving around bytes with zero copying.

@Kludex
Copy link
Sponsor Member

Kludex commented Apr 28, 2024

This is a PR without description, and there's a single discussion that mentioned this feature request (that is 4 years old) without reactions.

Also, this can be easily done by overriding the stream_response method. And that's what we've always recommended.

@adriangb
Copy link
Member

Feel free to revert it if you don’t agree. To me it made sense and was simple.

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.

None yet

3 participants