Skip to content

bpo-43219: shutil.copyfile, raise a less confusing exception instead of IsADirectoryError #27049

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

Jump to bottom
Merged
gpshead merged 3 commits into from
Jul 10, 2021
Merged

bpo-43219: shutil.copyfile, raise a less confusing exception instead of IsADirectoryError #27049

gpshead merged 3 commits into from
Jul 10, 2021

Conversation

@akulakov
Copy link
Contributor

@akulakov akulakov commented Jul 7, 2021 •
edited by bedevere-bot
Loading

@akulakov akulakov marked this pull request as draft July 7, 2021 02:30
@akulakov akulakov marked this pull request as ready for review July 8, 2021 13:42
@akulakov
Copy link
Contributor Author

akulakov commented Jul 8, 2021

Note that before merging, this should be run with builds on other OSes like Aix, FreeBSD, etc.

@gpshead gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 9, 2021
@bedevere-bot
Copy link

bedevere-bot commented Jul 9, 2021

🤖 New build scheduled with the buildbot fleet by @gpshead for commit 624093b 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 9, 2021
@gpshead gpshead self-assigned this Jul 9, 2021
@akulakov
Copy link
Contributor Author

akulakov commented Jul 9, 2021

For reviewing: note that the code block wrapped in try/except is unchanged except for indentation.

@gpshead gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 10, 2021
@bedevere-bot
Copy link

bedevere-bot commented Jul 10, 2021

🤖 New build scheduled with the buildbot fleet by @gpshead for commit b2a681b 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 10, 2021
gpshead
gpshead reviewed Jul 10, 2021
View reviewed changes
@gpshead gpshead added needs backport to 3.8 needs backport to 3.10 only security fixes type-bug An unexpected behavior, bug, or error and removed needs backport to 3.8 labels Jul 10, 2021
@gpshead gpshead merged commit 248173c into python:main Jul 10, 2021
@miss-islington
Copy link
Contributor

miss-islington commented Jul 10, 2021

Thanks @akulakov for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 10, 2021
@akulakov @miss-islington
bpo-43219: shutil.copyfile, raise a less confusing exception instead …
c264d9d 
…of IsADirectoryError (pythonGH-27049)

Fixes the misleading IsADirectoryError to be FileNotFoundError.
(cherry picked from commit 248173c)

Co-authored-by: andrei kulakov <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 10, 2021
@bedevere-bot
Copy link

bedevere-bot commented Jul 10, 2021

GH-27081 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 10, 2021
@akulakov @miss-islington
bpo-43219: shutil.copyfile, raise a less confusing exception instead …
274e595 
…of IsADirectoryError (pythonGH-27049)

Fixes the misleading IsADirectoryError to be FileNotFoundError.
(cherry picked from commit 248173c)

Co-authored-by: andrei kulakov <[email protected]>
@bedevere-bot
Copy link

bedevere-bot commented Jul 10, 2021

GH-27082 is a backport of this pull request to the 3.9 branch.

miss-islington added a commit that referenced this pull request Jul 10, 2021
@miss-islington @akulakov
bpo-43219: shutil.copyfile, raise a less confusing exception instead …
1577259 
…of IsADirectoryError (GH-27049)

Fixes the misleading IsADirectoryError to be FileNotFoundError.
(cherry picked from commit 248173c)

Co-authored-by: andrei kulakov <[email protected]>
miss-islington added a commit that referenced this pull request Jul 10, 2021
@miss-islington
[3.9] bpo-43219: shutil.copyfile, raise a less confusing exception in…
c89f0b2 
…stead of IsADirectoryError (GH-27049) (GH-27082)

Fixes the misleading IsADirectoryError to be FileNotFoundError.
(cherry picked from commit 248173c)


Co-authored-by: andrei kulakov <[email protected]>

Automerge-Triggered-By: GH:gpshead
@akulakov akulakov deleted the 43219-Shutil-copy-fix-is-a-dir-errors branch July 10, 2021 04:51
@akulakov
Copy link
Contributor Author

akulakov commented Jul 10, 2021

@gpshead Thanks for the review :)

@pablogsal pablogsal mentioned this pull request Jul 18, 2021
Closed
@Flamefire
Copy link

Flamefire commented Sep 17, 2021

This now raises a wrong FileNotFoundError when the src is a directory because only the dst existance is checked.

@akulakov
Copy link
Contributor Author

akulakov commented Sep 17, 2021

This now raises a wrong FileNotFoundError when the src is a directory because only the dst existance is checked.

FileNotFoundError is a common exception for "file or directory does not exist". If you feel it's wrongly raised in some scenario, please open a new issue on bugs.python.org and add an example and expected / observed output.

@Flamefire
Copy link

Flamefire commented Sep 17, 2021

But the path exists. It only is a directory. This PR changed the correct IsADirectoryError to a now incorrect FileNotFoundError for that case

@akulakov
Copy link
Contributor Author

akulakov commented Sep 17, 2021

@Flamefire I think you're right, I can reproduce it. I will reopen the issue.

@Flamefire
Copy link

Flamefire commented Sep 17, 2021

I opened https://bugs.python.org/issue45234 but mentioned the new failure in 43219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead approved these changes

Assignees

@gpshead gpshead

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@akulakov @bedevere-bot @miss-islington @Flamefire @gpshead @the-knights-who-say-ni