X Tutup
Skip to content

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

Merged
giampaolo merged 1 commit intopython:masterfrom
mbarkhau:copytree_ignore_regression
Jan 24, 2020
Merged

bpo-39390: fix argument types for ignore callback of shutil.copytree#18122
giampaolo merged 1 commit intopython:masterfrom
mbarkhau:copytree_ignore_regression

Conversation

@mbarkhau
Copy link
Contributor

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

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 reviewed changes
Lib/shutil.py Outdated
Copy link
Contributor

@giampaolo giampaolo Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mbarkhau mbarkhau Jan 22, 2020

Choose a reason for hiding this comment

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

I have made the requested changes; please review again.

Copy link
Contributor Author

@mbarkhau mbarkhau Jan 22, 2020

Choose a reason for hiding this comment

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

Sorry, I missed something for the test.

Copy link
Contributor Author

@mbarkhau mbarkhau Jan 22, 2020

Choose a reason for hiding this comment

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

Ok, I think it should be good now.

@bedevere-bot
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 copytree_ignore_regression branch 2 times, most recently from f18cf4f to 95953fd Compare January 22, 2020 19:29
giampaolo
giampaolo requested changes Jan 22, 2020
View reviewed changes
Lib/test/test_shutil.py Outdated
Copy link
Contributor

@giampaolo giampaolo Jan 22, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mbarkhau mbarkhau Jan 22, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mbarkhau mbarkhau Jan 22, 2020

Choose a reason for hiding this comment

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

I think the test should be ok now.

@mbarkhau mbarkhau force-pushed the copytree_ignore_regression branch from 95953fd to b0c0108 Compare January 22, 2020 19:43
@mbarkhau mbarkhau requested a review from giampaolo January 22, 2020 22:03
giampaolo
giampaolo requested changes Jan 23, 2020
View reviewed changes
Copy link
Contributor

@giampaolo giampaolo left a comment

Choose a reason for hiding this comment

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

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

Lib/test/test_shutil.py Outdated
Copy link
Contributor

@giampaolo giampaolo Jan 23, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mbarkhau mbarkhau Jan 23, 2020

Choose a reason for hiding this comment

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

Fair point.

Copy link
Contributor Author

@mbarkhau mbarkhau Jan 23, 2020

Choose a reason for hiding this comment

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

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

@mbarkhau mbarkhau force-pushed the copytree_ignore_regression branch from b0c0108 to eebcd7a Compare January 23, 2020 21:35
@mbarkhau mbarkhau requested a review from giampaolo January 23, 2020 21:46
@giampaolo giampaolo merged commit 8870433 into python:master Jan 24, 2020
@miss-islington
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

giampaolo commented Jan 24, 2020

Could you backport this to 3.8?

@mbarkhau
Copy link
Contributor Author

mbarkhau commented Jan 24, 2020

I'll have a look.

@bedevere-bot
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 copytree_ignore_regression branch January 24, 2020 15:43
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 pushed 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
cf9d005 
)
shihai1991 pushed 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 Awaiting requested review from giampaolo

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mbarkhau @bedevere-bot @miss-islington @giampaolo @the-knights-who-say-ni
X Tutup