Skip to content

gh-57531 add BufferedReader.read() return value check for non-blocking stream mode #105224

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

Closed
wants to merge 22 commits into from

Conversation

z764969689
Copy link
Contributor

gh-57531 add BufferedReader.read() return value check for non-blocking stream mode, simply opt the TextIOWrapper error raising.

@serhiy-storchaka
Copy link
Member

Please resolve conflicts and add tests. Also capitalize the first letter of a sentence in a NEWS entry.

@z764969689
Copy link
Contributor Author

z764969689 commented Jan 18, 2024

@serhiy-storchaka Conflicts resolved & NEWS entry edited. I'm wondering if I need to open another PR for the tests or just add them in this one.

@serhiy-storchaka
Copy link
Member

It is better to add new tests in the same PR. It makes easier to check that they fail without this change and pass with it.

@z764969689
Copy link
Contributor Author

I've added a simple test case and I found that the test case would fail the C implementation. Since I'm unfamiliar with the C code, might need some help from others to modify the C code.
BTW, not pretty clear on the implementation differences, is it okay that the error-raising logic varies between them?

@serhiy-storchaka
Copy link
Member

It is more complex issue than covered in this PR. If change the behavior of text file read() for non-blocking buffered readers, the same change should be done for non-blocking raw files. If change the behavior for "read-until-end", the same change should be done for partial read.

And of course the behavior of both C and Python implementation should be consistent.

Thank you for reviving this issue, I will look what can I do with this.

@z764969689 z764969689 deleted the fix-issue-57531 branch January 30, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants