gh-117829: add --include and --exclude flag to zipapp #120537
Conversation
…829' into implement-issue-117829
…829' into implement-issue-117829
Lib/test/test_zipapp.py
Outdated
| (source / 'bin' / 'baz').touch() | ||
| (source / '__main__.py').touch() | ||
|
|
||
| args = [str(source), '--include-pattern', r'.*\.py', '--exclude-pattern', r'.*z.*'] |
There was a problem hiding this comment.
Can self.tmpdir contain a z? If so, I believe this exclude pattern would ignore all files (since AFAICS it's matched against absolute paths, not against paths relative to the source directory)
There was a problem hiding this comment.
Thanks @godlygeek , note that this is fixed.
…829' into implement-issue-117829
| @@ -94,6 +94,17 @@ The following options are understood: | |||
| this case, any other options are ignored and SOURCE must be an archive, not a | |||
| directory. | |||
|
|
|||
| .. option:: --include-pattern | |||
There was a problem hiding this comment.
Tian suggested --include and --exclude; what is the reason for the -pattern suffix?
There was a problem hiding this comment.
I agree, --include-pattern is more verbose without adding any significant benefit (IMO).
| filter=None, compressed=False, include_pattern=None, | ||
| exclude_pattern=None): |
There was a problem hiding this comment.
It should be possible to directly translate the --include and --exclude CLI options into a filter function, thus leaving the create_archive API unchanged, no?
There was a problem hiding this comment.
Agreed, I'm a strong -1 on changing the create_archive API for this. The functionality is already available there, this should be purely a CLI change.
| @@ -316,6 +317,73 @@ def test_content_of_copied_archive(self): | |||
| with zipfile.ZipFile(new_target, 'r') as z: | |||
| self.assertEqual(set(z.namelist()), {'__main__.py'}) | |||
|
|
|||
| def test_create_archive_with_include_pattern(self): | |||
There was a problem hiding this comment.
This test isn't needed, as we shouldn't be changing create_archive.
| self.assertEqual(zf.namelist(), | ||
| ["__main__.py", "zed.py"]) | ||
|
|
||
| def test_create_archive_with_exclude_pattern(self): |
| self.assertEqual(zf.namelist(), | ||
| [".DS_Store", "bin/", "bin/baz", "bin/qux"]) | ||
|
|
||
| def test_create_archive_with_include_and_exclude_pattern(self): |
| def test_cmdline_create_with_exclude_pattern(self): | ||
| source = self.tmpdir / 'source' | ||
| source.mkdir() | ||
| (source / '.DS_Store').touch() |
There was a problem hiding this comment.
Let's not use .DS_Store as a test file. The whole point of the issue that started this is that MacOS has a bad habit of randomly adding such files - let's not risk our tests interacting badly with that OS misfeature.
| def test_cmdline_create_with_include_pattern(self): | ||
| source = self.tmpdir / 'source' | ||
| source.mkdir() | ||
| (source / '.DS_Store').touch() |
There was a problem hiding this comment.
Same as the comment below, let's not use .DS_Store as a test file.
| def test_cmdline_create_with_include_and_exclude_pattern(self): | ||
| source = self.tmpdir / 'source' | ||
| source.mkdir() | ||
| (source / '.DS_Store').touch() |
There was a problem hiding this comment.
... and here.
Can we not refactor these tests so they aren't so repetitive?
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
Hi, I'm a first time contributor to Python. I saw issue gh-117829 is interesting and took a stab at it. Currently it's a Work in Progress PR for others to see if my direction is good. Will add tests soon.