bpo-39390: fix argument types for ignore callback of shutil.copytree #18122

mbarkhau · 2020-01-22T17:38:25Z

The directory being copied.

In [21]: !tree
.
├── derp.txt
├── herp.txt
└── subdir
    └── subdireentry.txt

1 directory, 3 files

An ignore function for debugging.

In [23]: def _ignore(*args):
    ...:     print("ignore args:", args)
    ...:     return []

Python 3.7

In [25]: shutil.copytree(".", "/tmp/copytree37", ignore=_ignore)
ignore args: ('.', ['derp.txt', 'herp.txt', 'subdir'])
ignore args: ('./subdir', ['subdireentry.txt'])
Out[25]: '/tmp/copytree37'
In [26]: sys.version
Out[26]: '3.7.5 (default, Nov 20 2019, 09:21:52) \n[GCC 9.2.1 20191008]'

Python 3.8

In [32]: shutil.copytree(".", "/tmp/copytree38", ignore=_ignore)
ignore args: ('.', {'herp.txt', 'subdir', 'derp.txt'})
ignore args: (<DirEntry 'subdir'>, {'subdireentry.txt'})
Out[32]: '/tmp/copytree38'
In [33]: sys.version
Out[33]: '3.8.1 | packaged by conda-forge | (default, Jan  5 2020, 20:58:18) \n[GCC 7.3.0]'

https://bugs.python.org/issue39390

giampaolo · 2020-01-22T18:57:09Z

Lib/shutil.py

@@ -478,12 +478,12 @@ def _copytree(entries, src, dst, symlinks, ignore, copy_function,
                        continue
                    # otherwise let the copy occur. copy2 will raise an error
                    if srcentry.is_dir():
-                        copytree(srcobj, dstname, symlinks, ignore,
+                        copytree(srcname, dstname, symlinks, ignore,


This removes the speedup originally introduced. I think you can just do this:

ignored_names = ignore(os.fspath(src), [x.path for x in entries])

There should be a unit test which makes sure the callback function receives a string regardless of the type passed as input (str, pathlib.Path, os.DirEntry).

I have made the requested changes; please review again.

Sorry, I missed something for the test.

Ok, I think it should be good now.

bedevere-bot · 2020-01-22T18:57:36Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

giampaolo · 2020-01-22T19:34:06Z

Lib/test/test_shutil.py

+            finally:
+                shutil.rmtree(dst_dir)
+        finally:
+            shutil.rmtree(src_dir)


You can use self.addCleanup(shutil.rmtree, path) and avoid the try/finally blocks.

Hmm, actually from the looks of it, that's already part of self.mkdtemp.

I think the test should be ok now.

giampaolo

I added one last nitpick. Also, please add a NEWS entry with blurb.

giampaolo · 2020-01-23T10:08:15Z

Lib/test/test_shutil.py

+            self.assertEqual(len(names), len(set(names)))
+            for name in names:
+                self.assertIsInstance(name, str)
+            return []


I'd add a check to make sure this function is called 3 times. Something like:

def test_copytree_arg_types_of_ignore(self): def _ignore(src, names): ... flags.append(None) flags = [] ... # body self.assertEqual(len(flags), 3) # last line

Fair point.

Since it's called recursively, it's actually called a total of 9 times, but in any case it works.

miss-islington · 2020-01-24T14:51:31Z

Thanks @mbarkhau for the PR, and @giampaolo for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2020-01-24T14:51:32Z

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

giampaolo · 2020-01-24T14:52:32Z

Could you backport this to 3.8?

mbarkhau · 2020-01-24T15:25:02Z

I'll have a look.

bedevere-bot · 2020-01-24T15:40:26Z

GH-18168 is a backport of this pull request to the 3.8 branch.

…nGH-18122). (cherry picked from commit 8870433) Co-authored-by: mbarkhau <[email protected]>

)

the-knights-who-say-ni added the CLA signed label Jan 22, 2020

bedevere-bot added the awaiting review label Jan 22, 2020

mbarkhau mentioned this pull request Jan 22, 2020

bpo-39390: Update shutil.copytree doc - Add versionchanged 3.8 notice for ignore callable #18069

Closed

giampaolo requested changes Jan 22, 2020

View reviewed changes

bedevere-bot removed the awaiting review label Jan 22, 2020

bedevere-bot added the awaiting changes label Jan 22, 2020

mbarkhau force-pushed the copytree_ignore_regression branch 2 times, most recently from f18cf4f to 95953fd Compare January 22, 2020 19:29

giampaolo requested changes Jan 22, 2020

View reviewed changes

mbarkhau force-pushed the copytree_ignore_regression branch from 95953fd to b0c0108 Compare January 22, 2020 19:43

mbarkhau requested a review from giampaolo January 22, 2020 22:03

giampaolo requested changes Jan 23, 2020

View reviewed changes

bpo-39390 shutil: fix argument types for ignore callback

eebcd7a

mbarkhau force-pushed the copytree_ignore_regression branch from b0c0108 to eebcd7a Compare January 23, 2020 21:35

mbarkhau requested a review from giampaolo January 23, 2020 21:46

giampaolo merged commit 8870433 into python:master Jan 24, 2020

bedevere-bot removed the awaiting changes label Jan 24, 2020

giampaolo added the needs backport to 3.8 label Jan 24, 2020

bedevere-bot removed the needs backport to 3.8 label Jan 24, 2020

mbarkhau deleted the copytree_ignore_regression branch January 24, 2020 15:43

mbarkhau added a commit to mbarkhau/cpython that referenced this pull request Jan 27, 2020

[3.8] bpo-39390 shutil: fix argument types for ignore callback (pytho…

e08eb83

…nGH-18122). (cherry picked from commit 8870433) Co-authored-by: mbarkhau <[email protected]>

giampaolo pushed a commit that referenced this pull request Jan 27, 2020

[3.8] bpo-39390 shutil: fix argument types for ignore callback (GH-18122

cf9d005

)

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-39390 shutil: fix argument types for ignore callback (pythonGH-18122

9b2b939

)

Uh oh!

bpo-39390: fix argument types for ignore callback of shutil.copytree #18122

bpo-39390: fix argument types for ignore callback of shutil.copytree #18122

Uh oh!

Conversation

mbarkhau commented Jan 22, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giampaolo Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giampaolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jan 24, 2020

Uh oh!

miss-islington commented Jan 24, 2020

Uh oh!

giampaolo commented Jan 24, 2020

Uh oh!

mbarkhau commented Jan 24, 2020

Uh oh!

bedevere-bot commented Jan 24, 2020

Uh oh!

Uh oh!

mbarkhau commented Jan 22, 2020 •

edited by bedevere-bot

Loading

giampaolo Jan 22, 2020 •

edited

Loading