| msg153348 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2012-02-14 16:10 |
| I found a few possible bugs in tarfile: - “mode in 'raw'” can give false positives for '' or 'ra'. Most of the code also checks for “len(mode) > 1”, but I find clearer and safer to just use “mode in ('r', 'a', 'w')”. - To use the shadowed builtin “open”, tarfile uses both “import as” and a local alias “bltin_open = open”. However, the second idiom would break if tarfile were reloaded. - Error messages don’t say what the invalid mode was. (Error messages are not part of the language, but nonetheless maybe it’s best not to commit that to stable branches.) |
|
|
| msg153734 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2012-02-19 23:39 |
| Another one: now that shutil provides archiving operations, there is a circular dependency between tarfile and shutil. It does not cause problems*, as both modules use qualified names, but it may be a good thing to avoid import cascades for performance reasons. The single shutil function used by tarfile could be inlined, as I did in distutils2, or you may reject this idea. * Except in distutils2’s backport, see http://hg.python.org/distutils2/rev/c1b1b537196d |
|
|
| msg153954 - (view) |
Author: Lars Gustäbel (lars.gustaebel) * |
Date: 2012-02-22 14:26 |
| I updated your patch: - I removed the "import as" bit completely and changed all occurrences of _open() to builtins.open() which is more readable and explanatory. - I object to changing the error messages in the 3.2 branch due to backwards compatibility, although I left them in the patch for now. (I changed the style of %-formatting with a single item tuple in order to match the coding style of the rest of the module.) - I inlined the shutil.copyfileobj() method to remove the shutil import. |
|
|
| msg154026 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2012-02-23 01:08 |
| > I removed the "import as" bit completely and changed all occurrences of _open() to > builtins.open() which is more readable and explanatory. Truly. tokenize got a similar fix in ea260d393cde (without a test, so I think that here we don’t need one either); locale has a similar bug. > I object to changing the error messages in the 3.2 branch due to backwards compatibility, > although I left them in the patch for now. I removed these changes in the attached patch. I’ll make another patch for 3.3 for that. > (I changed the style of %-formatting with a single item tuple in order to match the coding > style of the rest of the module.) My reason was not style (I hate %-formatting with single-element tuples) but defensive coding, in case someone gives a tuple as argument. OTOH, that will just change the type of error they get for the same line, and the doc clearly says what is allowed, so it does not matter. > I inlined the shutil.copyfileobj() method to remove the shutil import. Great, this will eliminate a circular dependency I had in a shutil refactoring (I need to access tarfile.compression_formats, but the tarfile module is not ready when shutil gets imported), and also reduce the diff with the tarfile backport we have in distutils2. Tell me if you want me to commit. |
|
|
| msg181514 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-02-06 12:56 |
| The patch is desynchronized from current sources. |
|
|
| msg186739 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-04-13 16:06 |
| Éric, can you please update your patch? |
|
|
| msg186963 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2013-04-15 00:55 |
| I should be able to do that but can’t say when. |
|
|
| msg194333 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-08-04 08:16 |
| Here is updated for 3.4 patch. |
|
|
| msg195076 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2013-08-13 14:54 |
| Thanks, LGTM. |
|
|
| msg195127 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-08-14 07:37 |
| Lets push it. Lars? |
|
|
| msg228938 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2014-10-10 01:28 |
| The patch no longer applies again. |
|
|