#4035 enhancement closed fixed (fixed)
use python 2.6's streaming zipfile support instead of our own zipstream module
| Reported by: | kmike | Owned by: | lvh |
|---|---|---|---|
| Priority: | normal | Milestone: | |
| Component: | core | Keywords: | easy |
| Cc: | kmike, Thijs Triemstra | Branch: |
branches/streaming-zipfile-4035
branch-diff, diff-cov, branch-cov, buildbot |
| Author: | lvh |
Description
As I can see the goal of zipstream is to provide support for an incremental approach to unzipping files.
But standard zipfile has this support since python 2.6. ZipFile?.open() method returns file-like object which supports 'read' method with buffer size in parameters.
Implementation in zipfile is better than in zipstream. At least it works a lot faster because it don't have 'self.buffer = self.buffer[n:]' code which kills performance on big (even tens of Kb's) buffer sizes like zipstream version does.
I think is better to deprecate zipstream and copy implementation from new zipfile. Or make zipstream a backported zipfile wrapper.
Attachments (2)
Change History (17)
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
| Keywords: | easy added |
|---|---|
| Owner: | changed from Glyph to kmike |
| Summary: | deprecate or improve zipstream → use python 2.6's streaming zipfile support instead of our own zipstream module |
I agree, it would be good to use the improved implementation on python 2.6 (and using a direct backport is better, because then we only have one bit of code to maintain).
comment:3 Changed 9 years ago by
Please note that ZipPath.open is the main use-case for this code. Really, the whole module should have been an implementation detail of ZipPath in the first place.
comment:4 Changed 8 years ago by
| Owner: | changed from kmike to Andy Ruse |
|---|---|
| Status: | new → assigned |
Changed 8 years ago by
| Attachment: | 4035.patch added |
|---|
Modified zippath module to use the standard zipfile module if version is >= 2.6. Older versions will still use zipstream.
comment:5 Changed 8 years ago by
| Keywords: | review added |
|---|---|
| Owner: | Andy Ruse deleted |
| Status: | assigned → new |
comment:6 Changed 8 years ago by
| Keywords: | easy, review → easy review |
|---|
comment:7 Changed 8 years ago by
| Cc: | Thijs Triemstra added |
|---|---|
| Keywords: | review removed |
| Owner: | set to Andy Ruse |
This patch is missing tests and a news file, see TwistedDevelopment
Changed 8 years ago by
| Attachment: | 4035.patch2 added |
|---|
comment:8 Changed 8 years ago by
| Keywords: | review added |
|---|---|
| Owner: | Andy Ruse deleted |
I didn't write any new tests because the existing tests already cover the code that was modified. I've attached a new patch with a news file.
comment:9 Changed 8 years ago by
| Owner: | set to lvh |
|---|---|
| Status: | new → assigned |
comment:10 Changed 8 years ago by
(For other people watching this: please don't name your N-th patch blah.patchN, because trac no longer recognizes your thing as a patch)
comment:11 Changed 8 years ago by
| Keywords: | review removed |
|---|---|
| Status: | assigned → new |
Looks great, merging (with previous caveat).
comment:12 Changed 8 years ago by
| Author: | → lvh |
|---|---|
| Branch: | → branches/streaming-zipfile-4035 |
(In [31050]) Branching to 'streaming-zipfile-4035'
comment:13 Changed 8 years ago by
comment:14 Changed 8 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
See also #3666.