On Tue, Feb 24, 2015 at 10:50 AM, Paul Moore <p.f.moore@gmail.com> wrote:

On 24 February 2015 at 18:24, Guido van Rossum <guido@python.org> wrote:

...

Here's my review. I really like where this is going but I have a few questions and suggestions (I can't help myself :-).

Thanks.

...

[I sneaked a peek at the update you sent to peps@python.org.]

"Currently, pyyzer [5] and pex [6] are two tools known to exist." -> "... are two such tools."

It's not stated whether the archive names include the .pyz[w] extension or not (though I'm guessing it's not -- this should be stated).

No, I assumed that automatically adding extensions is not the norm. A lot of Windows tools do it, but I don't get the impression Unix tools do. I'll clarify the wording.

...

The naming of the functions feels inconsistent -- maybe pack(directory, target) -> create_archive(directory, archive), and set_interpreter() -> copy_archive(archive, new_archive)?

I'm OK with create_archive. But not so sure about copy_archive - the purpose of that function is solely to change the shebang line on an existing archive. It does so by copying, sure, but that's incidental. I see get_interpreter/set_interpreter as complementary "helper" APIs, and create_archive as the main API.

To me, the name set_<something>() so strongly evokes the idea of in-place modification that I cannot live with it. (I also assume that your code won't do the right thing if the output is the same as the input, right?)

...

Why no command-line equivalent for the other two methods? I propose the following interface: if there's only one positional argument, we're asking to print its shebang line; if there are two and the input position is an archive instead of a directory, we're copying. (In the future people will want an option to print more stuff, e.g. the main function or even a full listing.)

Mainly because I couldn't think of an API that didn't look muddled, or make the simple case of "I want to pack up a directory" look messy. But I'll think about your proposal - it sounds nice, I'll see how it fares when I try to implement it :-)

...

I've not seen the pkg.mod:fn notation before. Where is this taken from? Why can't it be pkg.mod.fn?

It's common usage in setuptools and things like tat - it's the "entry point" syntax used in packaging. I don't know of any reason why all-dotted notation wouldn't work, though. I'll ask around if there's any reason I'm not aware of.

OK, no need to change this then.

...

I'd specify that when the output argument is a file open for writing, it is the caller's responsibility to close the file. Also, can the file be a pipe? (I.e. are we using seek()/tell() or not?) And what about the input archive? Can that be a file open for reading?

I'll clarify all of these points. They are mostly "it can be whatever the zipfile module accepts", though, which isn't explicitly stated itself :-(

The input can't be a file object for pack/create_archive because it's a directory, not an archive, there. But it could be for set_interpreter. I just wasn't sure if it was worth adding. It's not hard to do, though.

I'm not sure what use case this is addressing anyway, but assuming there is one, you might as well go all the way (except for directories, obviously). -- --Guido van Rossum (python.org/~guido)