Skip to content

/ cpython

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.

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

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

Merged
merged 1 commit into from Jan 24, 2020
Merged

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

merged 1 commit into from Jan 24, 2020

Conversation

@mbarkhau
Copy link
Contributor

mbarkhau commented Jan 22, 2020 •
edited by bedevere-bot

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
@mbarkhau mbarkhau mentioned this pull request Jan 22, 2020
Closed
@giampaolo
giampaolo requested changes Jan 22, 2020
View changes
Lib/shutil.py Outdated
@@ -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 comment has been minimized.

Sign in to view
Copy link
@giampaolo

giampaolo Jan 22, 2020 •
edited

Contributor

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).

This comment has been minimized.

Sign in to view
Copy link
@mbarkhau

mbarkhau Jan 22, 2020

Author Contributor

I have made the requested changes; please review again.

This comment has been minimized.

Sign in to view
Copy link
@mbarkhau

mbarkhau Jan 22, 2020

Author Contributor

Sorry, I missed something for the test.

This comment has been minimized.

Sign in to view
Copy link
@mbarkhau

mbarkhau Jan 22, 2020

Author Contributor

Ok, I think it should be good now.
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Jan 22, 2020

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.
@mbarkhau mbarkhau force-pushed the mbarkhau:copytree_ignore_regression branch 2 times, most recently from f18cf4f to 95953fd Jan 22, 2020
@giampaolo
giampaolo requested changes Jan 22, 2020
View changes
Lib/test/test_shutil.py Outdated
finally:
shutil.rmtree(dst_dir)
finally:
shutil.rmtree(src_dir)

This comment has been minimized.

Sign in to view
Copy link
@giampaolo

giampaolo Jan 22, 2020

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@mbarkhau

mbarkhau Jan 22, 2020

Author Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@mbarkhau

mbarkhau Jan 22, 2020

Author Contributor

I think the test should be ok now.
@mbarkhau mbarkhau force-pushed the mbarkhau:copytree_ignore_regression branch from 95953fd to b0c0108 Jan 22, 2020
@mbarkhau mbarkhau requested a review from giampaolo Jan 22, 2020
@giampaolo
giampaolo requested changes Jan 23, 2020
View changes
Copy link
Contributor

giampaolo left a comment

I added one last nitpick. Also, please add a NEWS entry with blurb.
Lib/test/test_shutil.py
self.assertEqual(len(names), len(set(names)))
for name in names:
self.assertIsInstance(name, str)
return []

This comment has been minimized.

Sign in to view
Copy link
@giampaolo

giampaolo Jan 23, 2020

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@mbarkhau

mbarkhau Jan 23, 2020

Author Contributor

Fair point.

This comment has been minimized.

Sign in to view
Copy link
@mbarkhau

mbarkhau Jan 23, 2020

Author Contributor

Since it's called recursively, it's actually called a total of 9 times, but in any case it works.
@mbarkhau
bpo-39390 shutil: fix argument types for ignore callback
Loading status checks…
eebcd7a
@mbarkhau mbarkhau force-pushed the mbarkhau:copytree_ignore_regression branch from b0c0108 to eebcd7a Jan 23, 2020
@mbarkhau mbarkhau requested a review from giampaolo Jan 23, 2020
@giampaolo giampaolo merged commit 8870433 into python:master Jan 24, 2020
8 checks passed
8 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200123.60 succeeded
Details
bedevere/issue-number Issue number 39390 found
Details
bedevere/news News entry found in Misc/NEWS.d
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Jan 24, 2020

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

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Jan 24, 2020

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

giampaolo commented Jan 24, 2020

Could you backport this to 3.8?
@mbarkhau

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

mbarkhau commented Jan 24, 2020

I'll have a look.
mbarkhau added a commit to mbarkhau/cpython that referenced this pull request Jan 24, 2020
@mbarkhau
[3.8] bpo-39390 shutil: fix argument types for ignore callback (pytho…
778ba9c 
…nGH-18122).

(cherry picked from commit 8870433)

Co-authored-by: mbarkhau <mbarkhau@gmail.com>
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Jan 24, 2020

GH-18168 is a backport of this pull request to the 3.8 branch.
@mbarkhau mbarkhau deleted the mbarkhau:copytree_ignore_regression branch Jan 24, 2020
mbarkhau added a commit to mbarkhau/cpython that referenced this pull request Jan 27, 2020
@mbarkhau
[3.8] bpo-39390 shutil: fix argument types for ignore callback (pytho…
4c7cd9b 
…nGH-18122).

(cherry picked from commit 8870433)

Co-authored-by: mbarkhau <mbarkhau@gmail.com>
mbarkhau added a commit to mbarkhau/cpython that referenced this pull request Jan 27, 2020
@mbarkhau
[3.8] bpo-39390 shutil: fix argument types for ignore callback (pytho…
e08eb83 
…nGH-18122).

(cherry picked from commit 8870433)

Co-authored-by: mbarkhau <mbarkhau@gmail.com>
giampaolo added a commit that referenced this pull request Jan 27, 2020
@mbarkhau @giampaolo
[3.8] bpo-39390 shutil: fix argument types for ignore callback (GH-18122
Loading status checks…
cf9d005 
)
shihai1991 added a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@mbarkhau @shihai1991
bpo-39390 shutil: fix argument types for ignore callback (pythonGH-18122
9b2b939 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giampaolo giampaolo

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
5 participants
@mbarkhau @bedevere-bot @miss-islington @giampaolo @the-knights-who-say-ni
You can’t perform that action at this time.