#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 8 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 8 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 7 years ago by
Owner: | changed from kmike to Andy Ruse |
---|---|
Status: | new → assigned |
Changed 7 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 7 years ago by
Keywords: | review added |
---|---|
Owner: | Andy Ruse deleted |
Status: | assigned → new |
comment:6 Changed 7 years ago by
Keywords: | easy, review → easy review |
---|
comment:7 Changed 7 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 7 years ago by
Attachment: | 4035.patch2 added |
---|
comment:8 Changed 7 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 7 years ago by
Owner: | set to lvh |
---|---|
Status: | new → assigned |
comment:10 Changed 7 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 7 years ago by
Keywords: | review removed |
---|---|
Status: | assigned → new |
Looks great, merging (with previous caveat).
comment:12 Changed 7 years ago by
Author: | → lvh |
---|---|
Branch: | → branches/streaming-zipfile-4035 |
(In [31050]) Branching to 'streaming-zipfile-4035'
comment:13 Changed 7 years ago by
comment:14 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
See also #3666.