Skip to content

bpo-15450: Allow subclassing of filecmp.dircmp #23424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 23, 2020
Merged

bpo-15450: Allow subclassing of filecmp.dircmp #23424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Doc/library/filecmp.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,13 @@ The :class:`dircmp` class
.. attribute:: subdirs

A dictionary mapping names in :attr:`common_dirs` to :class:`dircmp`
objects.
instances (or MyDirCmp instances if this instance is of type MyDirCmp, a
subclass of :class:`dircmp`).

.. versionchanged:: 3.10
Previously entries were always :class:`dircmp` instances. Now entries
are the same type as *self*, if *self* is a subclass of
:class:`dircmp`.

.. attribute:: DEFAULT_IGNORES

Expand Down
9 changes: 6 additions & 3 deletions Lib/filecmp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@ class dircmp:
same_files: list of identical files.
diff_files: list of filenames which differ.
funny_files: list of files which could not be compared.
subdirs: a dictionary of dircmp objects, keyed by names in common_dirs.
subdirs: a dictionary of dircmp instances (or MyDirCmp instances if this
object is of type MyDirCmp, a subclass of dircmp), keyed by names
in common_dirs.
"""

def __init__(self, a, b, ignore=None, hide=None): # Initialize
Expand Down Expand Up @@ -185,14 +187,15 @@ def phase3(self): # Find out differences between common files
self.same_files, self.diff_files, self.funny_files = xx

def phase4(self): # Find out differences between common subdirectories
# A new dircmp object is created for each common subdirectory,
# A new dircmp (or MyDirCmp if dircmp was subclassed) object is created
# for each common subdirectory,
# these are stored in a dictionary indexed by filename.
# The hide and ignore properties are inherited from the parent
self.subdirs = {}
for x in self.common_dirs:
a_x = os.path.join(self.left, x)
b_x = os.path.join(self.right, x)
self.subdirs[x] = dircmp(a_x, b_x, self.ignore, self.hide)
self.subdirs[x] = self.__class__(a_x, b_x, self.ignore, self.hide)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use type(self) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really any good reason, I was copying @cjerdonek's patch and that's what he used. They evaluate to the same thing, right? I believe in Python 2 we would have needed to use .__class__, but now there is not that restriction. You're just thinking of style?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same thing I guess, both variants are equal.


def phase4_closure(self): # Recursively call phase4() on subdirectories
self.phase4()
Expand Down
53 changes: 44 additions & 9 deletions Lib/test/test_filecmp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ def setUp(self):
for dir in (self.dir, self.dir_same, self.dir_diff, self.dir_ignored):
shutil.rmtree(dir, True)
os.mkdir(dir)
subdir_path = os.path.join(dir, 'subdir')
os.mkdir(subdir_path)
if self.caseinsensitive and dir is self.dir_same:
fn = 'FiLe' # Verify case-insensitive comparison
else:
Expand Down Expand Up @@ -110,24 +112,33 @@ def test_cmpfiles(self):
"Comparing mismatched directories fails")


def _assert_lists(self, actual, expected):
"""Assert that two lists are equal, up to ordering."""
self.assertEqual(sorted(actual), sorted(expected))


def test_dircmp(self):
# Check attributes for comparison of two identical directories
left_dir, right_dir = self.dir, self.dir_same
d = filecmp.dircmp(left_dir, right_dir)
self.assertEqual(d.left, left_dir)
self.assertEqual(d.right, right_dir)
if self.caseinsensitive:
self.assertEqual([d.left_list, d.right_list],[['file'], ['FiLe']])
self._assert_lists(d.left_list, ['file', 'subdir'])
self._assert_lists(d.right_list, ['FiLe', 'subdir'])
else:
self.assertEqual([d.left_list, d.right_list],[['file'], ['file']])
self.assertEqual(d.common, ['file'])
self._assert_lists(d.left_list, ['file', 'subdir'])
self._assert_lists(d.right_list, ['file', 'subdir'])
self._assert_lists(d.common, ['file', 'subdir'])
self._assert_lists(d.common_dirs, ['subdir'])
self.assertEqual(d.left_only, [])
self.assertEqual(d.right_only, [])
self.assertEqual(d.same_files, ['file'])
self.assertEqual(d.diff_files, [])
expected_report = [
"diff {} {}".format(self.dir, self.dir_same),
"Identical files : ['file']",
"Common subdirectories : ['subdir']",
]
self._assert_report(d.report, expected_report)

Expand All @@ -136,9 +147,10 @@ def test_dircmp(self):
d = filecmp.dircmp(left_dir, right_dir)
self.assertEqual(d.left, left_dir)
self.assertEqual(d.right, right_dir)
self.assertEqual(d.left_list, ['file'])
self.assertEqual(d.right_list, ['file', 'file2'])
self.assertEqual(d.common, ['file'])
self._assert_lists(d.left_list, ['file', 'subdir'])
self._assert_lists(d.right_list, ['file', 'file2', 'subdir'])
self._assert_lists(d.common, ['file', 'subdir'])
self._assert_lists(d.common_dirs, ['subdir'])
self.assertEqual(d.left_only, [])
self.assertEqual(d.right_only, ['file2'])
self.assertEqual(d.same_files, ['file'])
Expand All @@ -147,6 +159,7 @@ def test_dircmp(self):
"diff {} {}".format(self.dir, self.dir_diff),
"Only in {} : ['file2']".format(self.dir_diff),
"Identical files : ['file']",
"Common subdirectories : ['subdir']",
]
self._assert_report(d.report, expected_report)

Expand All @@ -159,9 +172,9 @@ def test_dircmp(self):
d = filecmp.dircmp(left_dir, right_dir)
self.assertEqual(d.left, left_dir)
self.assertEqual(d.right, right_dir)
self.assertEqual(d.left_list, ['file', 'file2'])
self.assertEqual(d.right_list, ['file'])
self.assertEqual(d.common, ['file'])
self._assert_lists(d.left_list, ['file', 'file2', 'subdir'])
self._assert_lists(d.right_list, ['file', 'subdir'])
self._assert_lists(d.common, ['file', 'subdir'])
self.assertEqual(d.left_only, ['file2'])
self.assertEqual(d.right_only, [])
self.assertEqual(d.same_files, ['file'])
Expand All @@ -170,6 +183,7 @@ def test_dircmp(self):
"diff {} {}".format(self.dir, self.dir_diff),
"Only in {} : ['file2']".format(self.dir),
"Identical files : ['file']",
"Common subdirectories : ['subdir']",
]
self._assert_report(d.report, expected_report)

Expand All @@ -183,24 +197,45 @@ def test_dircmp(self):
"diff {} {}".format(self.dir, self.dir_diff),
"Identical files : ['file']",
"Differing files : ['file2']",
"Common subdirectories : ['subdir']",
]
self._assert_report(d.report, expected_report)

def test_dircmp_subdirs_type(self):
"""Check that dircmp.subdirs respects subclassing."""
class MyDirCmp(filecmp.dircmp):
pass
d = MyDirCmp(self.dir, self.dir_diff)
sub_dirs = d.subdirs
self.assertEqual(list(sub_dirs.keys()), ['subdir'])
sub_dcmp = sub_dirs['subdir']
self.assertEqual(type(sub_dcmp), MyDirCmp)

def test_report_partial_closure(self):
left_dir, right_dir = self.dir, self.dir_same
d = filecmp.dircmp(left_dir, right_dir)
left_subdir = os.path.join(left_dir, 'subdir')
right_subdir = os.path.join(right_dir, 'subdir')
expected_report = [
"diff {} {}".format(self.dir, self.dir_same),
"Identical files : ['file']",
"Common subdirectories : ['subdir']",
'',
"diff {} {}".format(left_subdir, right_subdir),
]
self._assert_report(d.report_partial_closure, expected_report)

def test_report_full_closure(self):
left_dir, right_dir = self.dir, self.dir_same
d = filecmp.dircmp(left_dir, right_dir)
left_subdir = os.path.join(left_dir, 'subdir')
right_subdir = os.path.join(right_dir, 'subdir')
expected_report = [
"diff {} {}".format(self.dir, self.dir_same),
"Identical files : ['file']",
"Common subdirectories : ['subdir']",
'',
"diff {} {}".format(left_subdir, right_subdir),
]
self._assert_report(d.report_full_closure, expected_report)

Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,7 @@ Ryan Coyner
Christopher A. Craig
Jeremy Craven
Laura Creighton
Nick Crews
Tyler Crompton
Simon Cross
Felipe Cruz
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2020-11-20-10-38-34.bpo-15450.E-y9PA.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Make :class:`filecmp.dircmp` respect subclassing. Now the
:attr:`filecmp.dircmp.subdirs` behaves as expected when subclassing dircmp.