bpo-22671: Clarify and test default read method implementations#4568
Conversation
In this case, you can specify the author when you do the Git commit. To modify the author of an existing author: |
|
I don't think that a NEWS entry is needed here: I added the "skip news" label. |
CuriousLearner
force-pushed
the
fix-issue22671
branch
from
f66b9d0 to
00397a1
Compare
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
The doc change LGTM, but I have requests on the newly added tests.
| def test_RawIOBase_read(self): | ||
| # Exercise the default RawIOBase.read() implementation (which calls | ||
| # readinto() internally). | ||
| # Exercise the default limited RawIOBase.read() implementation (which |
There was a problem hiding this comment.
I dislike "limited", I would prefer to write read(n) for "limited", and read() for "unlimited".
| ) | ||
| for test in tests: | ||
| with self.subTest(test): | ||
| method, avail, request, result = test |
There was a problem hiding this comment.
Can you try to compute result from avail and request? result = min(avail, result, 5), no?
There was a problem hiding this comment.
+1, we can do that. But I was thinking that expected_read_size when mentioned explicitly as it is done right now, would be better for future reference.
| def test_BufferedIOBase_readinto(self): | ||
| # Exercise the default BufferedIOBase.readinto() and readinto1() | ||
| # implementations (which call read() or read1() internally). | ||
| class Reader(self.BufferedIOBase): |
There was a problem hiding this comment.
Can you add an init() method taking avail as a parameter?
| method, avail, request, result = test | ||
| reader = Reader() | ||
| reader.avail = bytes(range(avail)) | ||
| buffer = bytearray((0x81,) * request) |
There was a problem hiding this comment.
Please use a constant instead of 0x81, like:
UNUSED_BYTE = 0x81
(define in this function, no need to declare it at the module scope.)
|
Oh, GitHub merged the two commits into a single one and chose to use Sanyam Khurana as the author. Hopefully, I wrote both names in the commit message ;-) (I edited manually the commit message before merging.) |
|
Thanks @CuriousLearner for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6. |
…onGH-4568) Original patch written by Martin Panter, enhanced by Sanyam Khurana. (cherry picked from commit 1b74f9b)
|
GH-4796 is a backport of this pull request to the 3.6 branch. |
6 participants
This patch is by @vadmium . I take no credits for this.
I've applied it on the top of the master branch.
https://bugs.python.org/issue22671