Skip to content

Conversation

@jeffreyrack
Copy link
Contributor

@jeffreyrack jeffreyrack commented Aug 8, 2017

Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the 2 noted points, the travis check failed with a report of whitespace issues, and the documentation needs to be updated.

Lib/zipapp.py Outdated

def create_archive(source, target=None, interpreter=None, main=None):
def create_archive(source, target=None, interpreter=None, main=None,
include_file=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation needs fixing here

Lib/zipapp.py Outdated
for child in source.rglob('*'):
arcname = child.relative_to(source).as_posix()
z.write(child, arcname)
if include_file is None or include_file(pathlib.Path(child)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The include_file check should take arcfile as argument - specifically it should be passed a path that is relative to the source directory.

@pfmoore
Copy link
Member

pfmoore commented Aug 8, 2017

Thanks for the PR on this - I've made a couple of comments that need to be addressed, but basically the change looks good.

I'm going to be unable to get back to this for a week or so, so if I don't comment any further for a while, don't worry - I will get back to it!

@pfmoore
Copy link
Member

pfmoore commented Aug 9, 2017

I've added a NEWS entry to your PR. Otherwise, this looks good to go.

@pfmoore pfmoore merged commit b811d66 into python:master Aug 9, 2017
@pfmoore
Copy link
Member

pfmoore commented Aug 9, 2017

Thanks for the contribution!

@jeffreyrack jeffreyrack deleted the 31072 branch October 7, 2019 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants