classification
Title: Allow dircmp.report() output stream to be customized
Type: enhancement Stage: needs patch
Components: Library (Lib) Versions: Python 3.4
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: cbc, chris.jerdonek, eli.bendersky
Priority: normal Keywords: easy, patch

Created on 2012-07-26 07:48 by chris.jerdonek, last changed 2012-08-01 02:08 by cbc.

Files
File name Uploaded Description Edit
issue-15454-1.patch cbc, 2012-07-31 18:06 Addition of tests for dircmp report methods. review
Messages (5)
msg166464 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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.
History
Date User Action Args
2012-08-01 02:08:56cbcsetmessages: + msg167071
2012-07-31 18:26:02chris.jerdoneksetmessages: + msg167025
2012-07-31 18:06:57cbcsetfiles: + issue-15454-1.patch
keywords: + patch
messages: + msg167022
2012-07-30 01:53:47cbcsetnosy: + cbc
2012-07-26 23:25:35chris.jerdoneksetmessages: + msg166528
2012-07-26 16:37:43amaury.forgeotdarcsetstage: needs patch
2012-07-26 07:48:51chris.jerdonekcreate