| msg141856 - (view) |
Author: Hynek Schlawack (hynek) *  |
Date: 2011-08-10 10:09 |
| While working on #12715 I noticed that the tests of shutil aren't exactly consistent concerning reading and writing files. There were no less than two function to read files (one of them not being used at all) and two methods to write them. Additionally lots of code simply reads/writes by hand. I've unified the functionality in module functions read_file and write_file which can be told to open the file as binaries. |
|
|
| msg141894 - (view) |
Author: Petri Lehtinen (petri.lehtinen) * |
Date: 2011-08-11 06:39 |
| I'd call os.path.join() in the test functions rather than in read_file() and write_file(). This makes it easier to understand what the test is doing without looking at the code of read_file() and write_file(). Otherwise, looks good to me, and I think this would be useful cleanup. |
|
|
| msg141896 - (view) |
Author: Hynek Schlawack (hynek) * |
Date: 2011-08-11 07:39 |
| I tend to agree on public APIs, however in this case of a helper function the use case with a join is really really common so this extra function comes in very handy. I also kept it using lists, so it's more obvious than tuples. JFTR it wasn't my idea, so I'm not defensive about my own idea here. :) I just re-implemented it for read_file b/c it's really handy and saves a lot of typing. |
|
|
| msg141908 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-08-11 14:57 |
| I’ll make one change before committing: Lib/test/test_shutil.py:69: if isinstance(path, (list, tuple)): Using a list for path components does not make sense. I have changed a similar helper function in packaging to allow only tuples. Petri: these helper functions are all about convenienve. I would reject a patch for a function just doing open+read, but here I think that doing os.path.join+open+read is worth a function. We use such helpers all the time in packaging tests and it helps reducing boilerplate, without being very hard to understand. |
|
|
| msg141914 - (view) |
Author: Petri Lehtinen (petri.lehtinen) * |
Date: 2011-08-11 17:46 |
| Éric Araujo wrote: > Petri: these helper functions are all about convenienve. I would > reject a patch for a function just doing open+read, but here I think > that doing os.path.join+open+read is worth a function. We use such > helpers all the time in packaging tests and it helps reducing > boilerplate, without being very hard to understand. Ok, sounds reasonable. |
|
|
| msg141942 - (view) |
Author: Hynek Schlawack (hynek) * |
Date: 2011-08-12 08:35 |
| Eric, just to be clear: Are you making this list->tuple change or should I fix the patch? |
|
|
| msg141962 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-08-12 15:25 |
| “I’ll make one change before committing” :) |
|
|
| msg141979 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2011-08-12 17:56 |
| New changeset d52a1199d3f0 by Éric Araujo in branch 'default': Clean up test_shutil, to facilitate upcoming improvements (#12721). http://hg.python.org/cpython/rev/d52a1199d3f0 |
|
|
| msg141980 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-08-12 18:02 |
| I made more changes (see the changeset) and committed only to 3.3, as we try to refrain from cleanup/cosmetic changes in stable branches (you never know what will cause a bug), and as you wanted this cleanup prior to work on a 3.3-only patch. |
|
|
| msg142639 - (view) |
Author: Hynek Schlawack (hynek) * |
Date: 2011-08-21 19:15 |
| While writing my tests I realized, it would be really useful to make write_file() return the path it wrote to. I need the concatenated filenames most of the time, so I'm getting blocks of: fn = os.path.join(x,y) write_file(fn, 'contents') I'd prefer: fn = write_file((x,y), 'contents') Any thoughts? IMHO a write_file that concats path but doesn't return it is only useful in the tree-functions. |
|
|
| msg142776 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-08-22 23:33 |
| > While writing my tests I realized, it would be really useful to make > write_file() return the path it wrote to. Sure, please open another 3.3 report for this. |
|
|
| msg142796 - (view) |
Author: Hynek Schlawack (hynek) * |
Date: 2011-08-23 08:32 |
| Done in Issue12824. |
|
|