bpo-41352: Raise UnsupportedOperation for FileIO.readall() in "w" mode#21568
bpo-41352: Raise UnsupportedOperation for FileIO.readall() in "w" mode#21568evanWang1409 wants to merge 5 commits intopython:mainfrom
Conversation
|
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! |
check self->readable added tests for FileIO.read and FileIO.readall
iritkatriel
left a comment
iritkatriel
left a comment
There was a problem hiding this comment.
I ran the tests with and without the code change. The read test passed with and without the patch, the readall test failed without and passed with. So this seems correct.
What does readall do currently on a file which was opened with "w"? Does it sometimes work?
|
You need to add a news entry to this PR. You can created it with this form: https://blurb-it.herokuapp.com/add_blurb Look at some merged PRs to see what they look like. |
|
Hi @iritkatriel. I worked with @evanWang1409 to submit this patch and can answer your questions. The
|
Lib/test/test_fileio.py
Outdated
| w = os.fdopen(w, "w") | ||
| w.write("hello") | ||
| w.close() |
There was a problem hiding this comment.
Please use a with statement.
Lib/test/test_fileio.py
Outdated
| w = os.fdopen(w, "w") | ||
| w.write("hello") | ||
| w.close() |
There was a problem hiding this comment.
Please use a with statement.
| with self.assertRaises(_io.UnsupportedOperation): | ||
| f.read() | ||
|
|
||
| def testReadallWithWritingMode(self): |
There was a problem hiding this comment.
These new test methods have too much duplicated code.
There was a problem hiding this comment.
I (or @evanWang1409) can pull out the pipe setup into a separate method, if you would like. Otherwise I'm not sure what else is the right direction here. It's not clear what abstraction would make sense given that there are only two callers right now and this is a small test case.
Modules/_io/fileio.c
Outdated
| if (!self->readable) | ||
| return err_mode("reading"); |
There was a problem hiding this comment.
The PEP 7 style guide states that "all new C code requires braces".
|
Unfortunately can't checkout: My clone was up to date yesterday and local copy, not having issues with other PRs. |
|
The following commit authors need to sign the Contributor License Agreement: |
Signed CLA. Will come back and see if this fix is still needed. |
|
Another fix for this issue has been merged to |
|
Thank you for being persistent about it. I did not have the appetite to pursue it getting merged. |
9 participants
check self->readable
added tests for FileIO.read and FileIO.readall
https://bugs.python.org/issue41352