Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-38971: open file in codecs.open() closes if exception raised #17666
Conversation
This comment has been minimized.
This comment has been minimized.
the-knights-who-say-ni
commented
Dec 19, 2019
|
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 usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: 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
force-pushed the
caporta:bpo-38971-fix-codecs-open
branch
from
96e88dc
to
f6b7759
|
The fix LGTM, but I have some comments about the test. |
| @@ -1154,6 +1154,19 @@ def test_stream_bare(self): | |||
| got = ostream.getvalue() | |||
| self.assertEqual(got, unistring) | |||
|
|
|||
| class FileClosesOnRaiseTest(unittest.TestCase): | |||
This comment has been minimized.
This comment has been minimized.
serhiy-storchaka
Dec 21, 2019
Member
I think there is no need to add a new class for this test. You can add it in CodecsModuleTest.
| mock_open = mock.mock_open() | ||
| mock_lookup = mock.Mock(side_effect=Exception) | ||
|
|
||
| with mock.patch('codecs.lookup', mock_lookup): |
This comment has been minimized.
This comment has been minimized.
serhiy-storchaka
Dec 21, 2019
Member
Instead of patching codecs.lookup, just pass an invalid encoding.
|
|
||
| with mock.patch('codecs.lookup', mock_lookup): | ||
| with mock.patch('builtins.open', mock_open) as file: | ||
| with self.assertRaises(Exception): |
This comment has been minimized.
This comment has been minimized.
serhiy-storchaka
Dec 21, 2019
Member
Exception is too wide. Check that more specific LookupError is raised.
- 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
This comment has been minimized.
This comment has been minimized.
|
@serhiy-storchaka addressed the above comments, should be ready for re-review. thanks |
caporta commentedDec 19, 2019
•
edited by bedevere-bot
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