| msg166464 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2012-07-26 07:48 |
| Currently, filecmp.dircmp's report(), report_partial_closure(), and report_full_closure() methods all only allow printing to stdout. This issue is to provide some way for the caller to control the stream to which these methods print their output (e.g. a 'stream' argument on the three methods). This suggestion was made by Eli in the discussion for issue 15269. |
|
|
| msg166528 - (view) |
Author: Chris Jerdonek (chris.jerdonek) * |
Date: 2012-07-26 23:25 |
| For discussion purposes, I'd like to mention an alternate approach. (I haven't yet formed an opinion on what approach is preferable.) Python's built-in print() function accepts more than just a 'file' keyword argument: http://docs.python.org/dev/library/functions.html#print In addition, because adding a stream argument would require modifying the signatures of three different methods, adding a stream argument might not set us on the right path if we'd like to add support for more arguments to print() in the future (or add additional methods that print output alongside the existing reporting methods). Another approach is to add a print() method to the dircmp class and have the report methods call self.print() instead of print(). With this approach, callers wanting a different output stream could subclass dircmp and override the print method. This approach also seems like it would offer more flexibility. |
|
|
| msg167022 - (view) |
Author: Chris Calloway (cbc) * |
Date: 2012-07-31 18:06 |
| At the PyOhio sprints, I noticed this issue and found it interesting for low hanging fruit. First I went to make tests for this enhancement, only to find that report(), report_partial_closure(), and report_full_closure did not already have tests. In adding tests for these methods, I also found that dircmp is only tested for one directory level, leaving out coverage for a pretty wide swath of dircmp. The first patch I'm submitting here is just for the report methods for the level of dircmp testing which already exists. A subsequent patch will test the report methods a couple of directory levels deep to get full coverage for report_full_closure(). Even if this enhancement is not accepted, the report methods do need tests. I'm putting this patch here in advance of any other work to get feedback on the flavor of tests I have given the report methods on the existing one directory level of dircmp testing before proceeding with any deeper testing. The tests in this patch all pass with the existing filecmp code without adding any significant execution time to test_filecmp. Any deeper testing of dircmp will also have to not be necessarily comprehensive, as the permutations of test inputs start to become too time-consuming. setUp and tearDown may also need some refactoring for deeper testing. Should adding tests for the report methods be a separate issue from this enhancement? |
|
|
| msg167025 - (view) |
Author: Chris Jerdonek (chris.jerdonek) * |
Date: 2012-07-31 18:26 |
| > Should adding tests for the report methods be a separate issue from this enhancement? Yes, I would create separate issues for this. I would even recommend separating this into one issue for the report() methods (could be done first), and another issue for adding tests for additional directory levels (see additional note below). > Any deeper testing of dircmp will also have to not be necessarily comprehensive, as the permutations of test inputs start to become too time-consuming. setUp and tearDown may also need some refactoring for deeper testing. For testing additional directory levels, you may want to wait until after issue 15403 is complete, which is slated for (hopefully) shortly after the 3.3 release. That issue will add functions for the easy creation of arbitrarily nested directories of files, etc. You will see I made a comment there about application to the filecmp tests. |
|
|
| msg167071 - (view) |
Author: Chris Calloway (cbc) * |
Date: 2012-08-01 02:08 |
| Thank you and the issue to add report method tests is issue 15518. |
|
|
| msg348650 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-07-29 12:02 |
| This issue is not newcomer friendly, I remove the easy keyword. |
|
|