| msg212007 - (view) |
Author: Matthias Klose (doko) *  |
Date: 2014-02-23 17:15 |
| shutil imports distutils in _call_external_zip just for the calling of an external command. This should be done using subprocess these days. |
|
|
| msg212010 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2014-02-23 17:20 |
| Does it pose an actual problem? |
|
|
| msg212209 - (view) |
Author: Ronald Oussoren (ronaldoussoren) * |
Date: 2014-02-25 20:11 |
| Why is _call_external_zip needed at all? The code says it is used when the zipfile module is not available, but that module is part of the stdlib and should always be available. |
|
|
| msg212660 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2014-03-03 19:39 |
| Agreed; this looks like a relic. zlib is optional, but zipfile is always in the standard library. |
|
|
| msg212696 - (view) |
Author: Derek Chiang (derekchiang93) * |
Date: 2014-03-04 08:51 |
| New contributor here. I'm submitting a patch; please let me know if I'm doing something wrong :) |
|
|
| msg212870 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2014-03-07 10:31 |
| Patch looks good to me. |
|
|
| msg214167 - (view) |
Author: A.M. Kuchling (akuchling) * |
Date: 2014-03-20 02:06 |
| Derek: thanks for your patch! However, did you run the test suite for the shutil module to verify that your change is correct? (The developer guide discusses running tests at http://docs.python.org/devguide/runtests.html ) |
|
|
| msg214168 - (view) |
Author: Derek Chiang (derekchiang93) * |
Date: 2014-03-20 02:11 |
| I didn't because the patch seemed trivial. I'm sure there are automated tests that will be run before the patch gets merged? |
|
|
| msg214169 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2014-03-20 02:16 |
| Not currently. Tests run automatically after a patch is merged, which is why patch authors should run tests (especially if they’re changing something missing tests, or adding a new feature and tests for it). |
|
|
| msg214266 - (view) |
Author: A.M. Kuchling (akuchling) * |
Date: 2014-03-20 19:42 |
| Yes, tests are only run after a change is committed and pushed into Mercurial; this is done by BuildBot https://www.python.org/dev/buildbot/ . So it's a good idea to run tests before submitting a patch or committing a change. No matter how trivial a change seems, it should always be tested first. Every programmer has a few stories of "this can't possibly fail" changes that fail, sometimes spectacularly. (One of mine: I rewrote some C string-handling code for a product that supported 4 or 5 different Unixes and processor architectures, tried it on one of them, and concluded it was fine. It segfaulted on exactly one architecture. Unfortunately this was discovered by a VP who was demoing to a customer at the time. I got a talking-to about that one.) Running the tests finds a simple problem: there's no longer an 'import zipfile' statement. I'll add the import inside the _make_zipfile() function. This is against PEP 8, strictly speaking, but it means importing shutil doesn't always import zipfile; it'll only import the module if it's actually needed. (I'll probably do the same for the import of tarfile.) Derek, thanks for your patch! |
|
|
| msg214270 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2014-03-20 20:11 |
| New changeset 681e20f8b717 by Andrew Kuchling in branch 'default': #20744: don't try running an external 'zip' in shutil.make_archive() http://hg.python.org/cpython/rev/681e20f8b717 |
|
|
| msg214288 - (view) |
Author: Derek Chiang (derekchiang93) * |
Date: 2014-03-20 21:54 |
| Cool, thanks for doing this! |
|
|