GeezerVison is showing github.com — Extract ZipInfo for archive functionality · Issue #123424 · python/cpython
Extract ZipInfo for archive functionality #123424DescriptionIn #123354, I found the need to copy code from zipfile into the test:
|
def for_name(cls, name, archive): |
|
""" |
|
Construct the same way that ZipFile.writestr does. |
|
|
|
TODO: extract this functionality and re-use |
|
""" |
|
self = cls(filename=name, date_time=time.localtime(time.time())[:6]) |
|
self.compress_type = archive.compression |
|
self.compress_level = archive.compresslevel |
|
if self.filename.endswith('/'): # pragma: no cover |
|
self.external_attr = 0o40775 << 16 # drwxrwxr-x |
|
self.external_attr |= 0x10 # MS-DOS directory flag |
|
else: |
|
self.external_attr = 0o600 << 16 # ?rw------- |
|
return self |
Let's instead extract that functionality in the zipfile module for re-use in the test (before it starts diverging).
Linked PRs
Activitypicnixz commented on Aug 28, 2024 @jaraco are you taking it or not? if not, I can take it jaraco commented on Aug 28, 2024 Originally, I was planning to do it, but then I realized it might be an interesting one for others. Have at it! serhiy-storchaka commented on Aug 28, 2024 See also #121402 that implements a similar feature.
I paused it because I am planning to try a different approach. obfusk commented on Sep 2, 2024 Nothing against extracting the functionality if it's useful elsewhere, but the test doesn't actually depend on the compression or any of the metadata set by for_name(), just the file name. Nothing else should matter to zipfile.Path here. So removing for_name() and just using zf.writestr(DirtyZipInfo("foo\\bar"), b"content") should work fine. jaraco commented on Sep 2, 2024 When I added the for_name to DirtyZipInfo, I was aiming to replace writestr without altering the way it worked. That is, every other test relies on writestr, so I wanted to construct an equivalent ZipInfo, assuming that whoever wrote writestr had reasons for defaulting the date_time, compression type, compression level, and external attrs. I wanted to avoid having to research the motivations for each of those concerns to see if they may apply to this specific case. I wanted to avoid the possibility that those attributes had a meaningful effect or worse did not have an effect today but would in the future. More importantly, I wanted to be sure that if writestr changed in the future to add additional behavior, this routine would inherit that same behavior. By reducing incidental variance, we improve reliability long-term. Reacted by FC (Fay) Stegerman obfusk commented on Sep 2, 2024 Passing a ZipInfo to writestr() will not set any of those attributes like passing only a filename (and thus having writestr() create the ZipInfo) will and is clearly supported as part of the current API. I rely on this regularly for Reproducible Builds where I need to set the attributes to specific values and using the current time would definitely be unacceptable.
And the fact that writestr() by default uses the current time means no two test runs ever use the exact same metadata, which is something I would generally try to avoid.
That said, even if I very much doubt any of the metadata would ever affect this specific test, and would consider that changing to be a bug, I agree that ensuring consistency wrt to writestr() behaviour between different tests is good. Just wanted to clarify the current situation :) picnixz commented on Dec 22, 2024 Removing my assignment on this task since I didn't continue working on the associated PR (and have to prioritize my backlog).
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|