classification
Title: shutil.copytree - 3.8 changed argument types of the ignore callback
Type: behavior Stage: resolved
Components: Documentation, Library (Lib) Versions: Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: giampaolo.rodola Nosy List: docs@python, giampaolo.rodola, mbarkhau, serhiy.storchaka, vstinner
Priority: normal Keywords: 3.8regression, patch

Created on 2020-01-19 20:40 by mbarkhau, last changed 2020-01-27 23:48 by giampaolo.rodola. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 18069 closed mbarkhau, 2020-01-19 21:04
PR 18122 merged mbarkhau, 2020-01-22 17:38
PR 18168 merged mbarkhau, 2020-01-24 15:40
Messages (14)
msg360271 - (view) Author: Manuel Barkhau (mbarkhau) * Date: 2020-01-19 20:40
In Python 3.8, the types of the parameters to the ignore callable appear to have changed.

Previously the `src` parameter was a string and the `names` parameter was a list of strings. Now the `src` parameter appears to be either a `pathlib.Path` or an `os.DirEntry`, while the `names` parameter is a set of strings.

I would suggest adding the following to the documentation https://github.com/python/cpython/blob/master/Doc/library/shutil.rst

   .. versionchanged:: 3.8
      The types of arguments to *ignore* have changed. The first argument 
      (the directory being visited) is a func:`os.DirEntry` or a 
      func:`pathlib.Path`; Previously it was a string. The second argument is
      a set of strings; previously it was a list of strings.
msg360300 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-01-20 08:31
This looks like a backward incompatible change in 3.8. Should not copytree convert arguments of the ignore callback to str and list correspondingly?
msg360303 - (view) Author: Manuel Barkhau (mbarkhau) * Date: 2020-01-20 09:04
> This looks like a backward incompatible change in 3.8.

Indeed. 

> Should not copytree convert arguments of the ignore callback to str and list correspondingly?

Well, since any existing code probably expects that behavior (or at least probably works if that is the case), I would be for such a change. I'm not sure what your policy is though. If there is recently written code that assumes the new types, are you OK with that breaking if it is changed back?

I guess since it's an undocumented breaking change, it shouldn't be too much of an issue.
msg360427 - (view) Author: Manuel Barkhau (mbarkhau) * Date: 2020-01-21 19:16
Is there anything I can do to help move this forward?
msg360431 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2020-01-21 20:23
> Should not copytree convert arguments of the ignore callback to str and list correspondingly?

It should. I think it makes sense to just do this for Python 3.8.2 instead of updating the doc.
msg360435 - (view) Author: Manuel Barkhau (mbarkhau) * Date: 2020-01-21 21:29
Unless somebody else wants to, I could have a go at an PR to update shutil.py
msg360437 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2020-01-21 22:31
Yes, thanks. Whoever got bit by this is either getting an exception or not the intended behavior (due to failed string comparison). I doubt anybody is relying on the new type checking since it's not documented. If they are, they are probably just doing:

    def callback(name, names):
        if not isinstance(name, str):  # bugfix 3.8
            name = name.name
        ...
msg360621 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2020-01-24 14:51
New changeset 88704334e5262c6cd395a0809d4ef810f33f3ca5 by Giampaolo Rodola (mbarkhau) in branch 'master':
bpo-39390 shutil: fix argument types for ignore callback (GH-18122)
https://github.com/python/cpython/commit/88704334e5262c6cd395a0809d4ef810f33f3ca5
msg360622 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2020-01-24 14:59
For completeness, a similar problem is present also on python < 3.8 if passing a pathlib.Path type as *src*: the callback function will receive a pathlib.Path type once, and then string types.
msg360624 - (view) Author: Manuel Barkhau (mbarkhau) * Date: 2020-01-24 15:44
> For completeness, a similar problem is present also on python < 3.8

Fair point. I'll have a look.
msg360625 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-01-24 15:50
> For completeness, a similar problem is present also on python < 3.8 if passing a pathlib.Path type as *src*

I do not think this is a problem. If you pass a string, you will get a string, so existing code will continue to work as before. The only problem if you pass a string, but get an unexpected type.
msg360626 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2020-01-24 15:58
I don't think we need to change anything on < 3.8, but 3.8 and 3.9 will always convert *src* to str via os.fspath(), which IMO is more consistent (e.g. os.path.* functions and others do the same).
msg360627 - (view) Author: Manuel Barkhau (mbarkhau) * Date: 2020-01-24 16:02
> If you pass a string, you will get a string, so existing code will continue to work as before.

Somebody might have code that is running against a flat directory and have written their ignore function expecting to get a pathlib.Path, because that's the only case they encountered. This change would break their code and so would an upgrade to 3.9 with the patch that was just merged.
msg360825 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2020-01-27 23:46
New changeset cf9d00554715febf21cf94950da4f42723b80498 by Giampaolo Rodola (mbarkhau) in branch '3.8':
[3.8] bpo-39390 shutil: fix argument types for ignore callback (GH-18122)
https://github.com/python/cpython/commit/cf9d00554715febf21cf94950da4f42723b80498
History
Date User Action Args
2020-01-27 23:48:24giampaolo.rodolasetstatus: open -> closed
assignee: docs@python -> giampaolo.rodola
resolution: fixed
stage: patch review -> resolved
2020-01-27 23:46:33giampaolo.rodolasetmessages: + msg360825
2020-01-24 16:02:12mbarkhausetmessages: + msg360627
2020-01-24 15:58:01giampaolo.rodolasetmessages: + msg360626
2020-01-24 15:50:47serhiy.storchakasetmessages: + msg360625
2020-01-24 15:44:20mbarkhausetmessages: + msg360624
2020-01-24 15:40:25mbarkhausetpull_requests: + pull_request17554
2020-01-24 14:59:56giampaolo.rodolasetmessages: + msg360622
2020-01-24 14:51:25giampaolo.rodolasetmessages: + msg360621
2020-01-22 17:38:27mbarkhausetpull_requests: + pull_request17509
2020-01-21 22:38:25giampaolo.rodolasetnosy: + vstinner
2020-01-21 22:31:20giampaolo.rodolasetmessages: + msg360437
2020-01-21 21:29:39mbarkhausetmessages: + msg360435
2020-01-21 20:23:45giampaolo.rodolasetmessages: + msg360431
2020-01-21 20:13:36serhiy.storchakasetnosy: + giampaolo.rodola
2020-01-21 19:16:09mbarkhausetmessages: + msg360427
2020-01-20 22:46:49ned.deilysetkeywords: + patch
2020-01-20 22:45:54ned.deilysetkeywords: + 3.8regression, - patch
2020-01-20 09:04:32mbarkhausetmessages: + msg360303
2020-01-20 08:31:24serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg360300
2020-01-19 21:04:09mbarkhausetkeywords: + patch
stage: patch review
pull_requests: + pull_request17462
2020-01-19 20:47:19mbarkhausettitle: shutil.copytree - ignore callback behaviour change -> shutil.copytree - 3.8 changed argument types of the ignore callback
2020-01-19 20:40:46mbarkhaucreate