Skip to content

bpo-38971: open file in codecs.open() closes if exception raised #17666

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

Merged
merged 4 commits into from
Mar 2, 2020

Conversation

caporta
Copy link
Contributor

@caporta caporta commented Dec 19, 2019

https://bugs.python.org/issue38971

Open issue in the BPO indicated a desire to make the implementation of
codecs.open() at parity with io.open(), which implements a try/except to
assure file stream gets closed before an exception is raised.

https://bugs.python.org/issue38971

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@caporta

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Open issue in the BPO indicated a desire to make the implementation of
codecs.open() at parity with io.open(), which implements a try/except to
assure file stream gets closed before an exception is raised.
@caporta caporta force-pushed the bpo-38971-fix-codecs-open branch from 96e88dc to f6b7759 Compare December 19, 2019 22:45
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

The fix LGTM, but I have some comments about the test.

- Move spec out of seperate class and into CodecsModuleTest
- Pass invalid encoding to force exception rather than mock
- Specify that a LookupError is raised specifically
@caporta
Copy link
Contributor Author

caporta commented Dec 22, 2019

@serhiy-storchaka addressed the above comments, should be ready for re-review. thanks

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Thank you. Yet a nitpick to tests.

Prefer positional args to keywords in spec

Co-Authored-By: Serhiy Storchaka <[email protected]>
@caporta caporta requested review from serhiy-storchaka and removed request for serhiy-storchaka February 14, 2020 04:13
@caporta
Copy link
Contributor Author

caporta commented Feb 27, 2020

@serhiy-storchaka would it be possible to get a re-review of this PR? Thanks in advance!

@serhiy-storchaka serhiy-storchaka merged commit 2565ede into python:master Mar 2, 2020
@serhiy-storchaka serhiy-storchaka added needs backport to 3.7 type-bug An unexpected behavior, bug, or error labels Mar 2, 2020
@miss-islington
Copy link
Contributor

Thanks @caporta for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-18733 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 2, 2020
…thonGH-17666)

Open issue in the BPO indicated a desire to make the implementation of
codecs.open() at parity with io.open(), which implements a try/except to
assure file stream gets closed before an exception is raised.
(cherry picked from commit 2565ede)

Co-authored-by: Chris A <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 2, 2020
…thonGH-17666)

Open issue in the BPO indicated a desire to make the implementation of
codecs.open() at parity with io.open(), which implements a try/except to
assure file stream gets closed before an exception is raised.
(cherry picked from commit 2565ede)

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

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

miss-islington added a commit that referenced this pull request Mar 2, 2020
…-17666)

Open issue in the BPO indicated a desire to make the implementation of
codecs.open() at parity with io.open(), which implements a try/except to
assure file stream gets closed before an exception is raised.
(cherry picked from commit 2565ede)

Co-authored-by: Chris A <[email protected]>
miss-islington added a commit that referenced this pull request Mar 2, 2020
…-17666)

Open issue in the BPO indicated a desire to make the implementation of
codecs.open() at parity with io.open(), which implements a try/except to
assure file stream gets closed before an exception is raised.
(cherry picked from commit 2565ede)

Co-authored-by: Chris A <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants