Repository navigation
Fix .as_bytes() producing different results to write() - #14363
Dreamsorcerer wants to merge 14 commits into
Conversation
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #14363 +/- ##
========================================
Coverage 99.12% 99.12%
========================================
Files 135 135
Lines 53661 53809 +148
Branches 2737 2750 +13
========================================
+ Hits 53191 53340 +149
Misses 356 356
+ Partials 114 113 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Merging this PR will not alter performance1 warning
Comparing Footnotes
|
| """Return bytes representation of the multipart data.""" | ||
| buffer = bytearray() | ||
| await self._write(None, True, buffer) | ||
| return bytes(buffer) |
There was a problem hiding this comment.
@bdraco Do you think we should just change the return to bytearray and avoid the bytes allocation?
There was a problem hiding this comment.
Keep bytes. Public API documented as bytes. bytearray unhashable, fails isinstance(x, bytes) checks in callers. Final bytes(buffer) copy: one memcpy, negligible next to QP/base64 encoding cost. Alternative if copy matters: collect list[bytes], b"".join at end (single allocation, as bdraco noted).
Co-authored-by: Sam Bull <aa6bs0@sambull.org>
|
@aiolibsbot review |
PR Review — Fix .as_bytes() producing different results to write()Solid fix with one deprecation-leak to close before merge. The core change is right: routing
🟡 Important
1. Deprecation warning fires from aiohttp's own call path in web.Response.text
|
Not clear exactly what to do with this. As an attribute, it can't do async. I can suppress the deprecation for now, but not clear what should happen once .decode() is removed. |
Fixes #13495.