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
BufferedReader.read1(size) signature incompatible with BufferedIOBase.read1(size=-1) #67403
Comments
I am trying to make LZMAFile (which implements BufferedIOBase) use a BufferedReader in read mode. However this broke test_lzma.FileTestCase.test_read1_multistream(), which calls read1() with the default size argument. This is because BufferedReader.read1() does not accept size=-1: >>> stdin = open(0, "rb", closefd=False)
>>> stdin
<_io.BufferedReader name=0>
>>> stdin.read1() # Parameter is mandatory
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: read1() takes exactly 1 argument (0 given)
>>> stdin.read1(-1) # Does not accept the BufferedIOBase default
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: read length must be positive
>>> stdin.read1(0) # Technically not positive
b'' Also, the BufferedIOBase documentation does not say what the size=-1 value means, only that it reads and returns up to -1 bytes. |
Looking at the test suite:
It seems the most consistent way forward would be to:
|
Looking at this again, I think a less intrusive way forward would be to:
Relaxing the read1() signature would allow wider or easier use of BufferedReader, e.g. to implement HTTPResponse as I suggested in bpo-26499. The advantage would be using existing code that is well tested, used, optimized, etc, rather than a custom BufferedIOBase implementation which for the HTTP case is buggy. |
Shouldn't read1(-1) be the same as read1(sys.maxsize)? I.e. read from a buffer without limit. |
Calling BufferedReader.read1(sys.maxsize) gives me a MemoryError. Making read1(-1) equivalent to read1(sys.maxsize) only makes sense where the return value already has a predetermined size, and only a limited buffer needs to be allocated. Another interpretation is to return an arbitrary, modest buffer size. This is what I ended up doing with LZMAFile.read1() in bpo-23529: return no more than 8 KiB. It is not equivalent to sys.maxsize because more than 8 KiB is possible if you ask for it. HTTPResponse (for non-chunked responses) is similar, but uses a default of 16 KiB. |
This sounds reasonable to me. |
Okay here is a patch implementing read1(-1) in BufferedReader and BytesIO (my original proposal). Other changes:
|
New changeset b6886ac88e28 by Martin Panter in branch 'default': |
New changeset d4fce64b1c65 by Martin Panter in branch 'default': |
I don't understand this change: the default size is documented as -1. Did I miss something? + If *size* is −1 (the default) Is the "−" character deliberate in "−1"? I would expected |
The minus sign might have been deliberate at some stage, but I realize it is more accessible etc if it was ASCII -1, so I will change that. The idea of changing the signature is to avoid people thinking it accepts a keyword argument. See e.g. bpo-25030 for seek(offset, whence=SEEK_SET), bpo-14586 for truncate(size=None). |
2016-10-21 5:17 GMT+02:00 Martin Panter <report@bugs.python.org>:
Ah. Maybe we should modify the code to accept a keyword argument? :-) Or we might use the strange "read1(\, size=1)" syntax of Argument Clinic. |
Modifying the API to accept a keyword argument is not worthwhile IMO. That means modifying modules like http.client and zipfile (which use the wrong keyword name), and user-defined implementations may become incompatible. The question of documentation is discussed more in bpo-23738. I think one or two people were concerned about using the Arg Clinic slash. |
New changeset 2af6d94de492 by Martin Panter in branch 'default': |
Misc/NEWS
so that it is managed by towncrier #552Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: